Do not trim unsafe commits when open readonly engine #41041
Conversation
dnhatn
added
>enhancement
:Distributed Indexing/Distributed
|
Pinging @elastic/es-distributed |
dnhatn
changed the title
| + "] from last commit does not match global checkpoint [" + globalCheckpoint + "]"); | ||
| } | ||
| } | ||
| ensureMaxSeqNoEqualsToGlobalCheckpoint(seqNoStats); |
There was a problem hiding this comment.
I am working to make this check allow gaps in sequence numbers.
| + "] but got " + getRetentionLeases(); | ||
| trimUnsafeCommits(); | ||
| // don't trim anything if we are going to open a read only engine. | ||
| if (engineFactory.isReadOnlyEngineFactory() == false) { |
There was a problem hiding this comment.
I think we should add an assertion below that ensures that if it's returning true that it's a subclass of ReadOnlyEngine.
henningandersen
left a comment
henningandersen
left a comment
There was a problem hiding this comment.
Left one comment, I would prefer to keep the EngineFactory as is if possible.
I am not enough up to speed on the why closing a follower index is problematic wrt. read-only indices trimming indexes, will reach out to you about this.
| trimUnsafeCommits(); | ||
| // don't trim anything if we are going to open a read only engine. | ||
| if (engineFactory.isReadOnlyEngineFactory() == false) { | ||
| trimUnsafeCommits(); |
There was a problem hiding this comment.
Could we not simply move trimUnsafeCommits() into InternalEngine constructor? As far as I can see, we always trim right before constructing engine.
It would move trim inside the mutex, but I think that is not a problem.
If we do that, we can avoid the new method on EngineFactory. I think the trimming logically belongs with the engine, not the factory.
There was a problem hiding this comment.
+1 to move trimUnsafeCommits to the InternalEngine.
|
@s1monw and @henningandersen I've moved trimUnsafeCommits to InternalEngine. It's nicer I think. Can you please have another look? Thank you! |
henningandersen
left a comment
henningandersen
left a comment
There was a problem hiding this comment.
LGTM.
Thanks @dnhatn
s1monw
left a comment
s1monw
left a comment
There was a problem hiding this comment.
looks much better! thanks
|
@s1monw @henningandersen thanks for reviewing. |
Today we always trim unsafe commits (whose max_seq_no >= global checkpoint) before starting a read-write or read-only engine. This is mandatory for read-write engines because they must start with the safe commit. This is also fine for read-only engines since most of the cases we should have exactly one commit after closing an index (trimming is a noop). However, this is dangerous for following indices which might have more than one commits when they are being closed. With this change, we move the trimming logic to the ctor of InternalEngine so we won't trim anything if we are going to open a read-only engine.
Today we always trim unsafe commits (whose max_seq_no >= global checkpoint) before starting a read-write or read-only engine. This is mandatory for read-write engines because they must start with the safe commit. This is also fine for read-only engines since most of the cases we should have exactly one commit after closing an index (trimming is a noop). However, this is dangerous for following indices which might have more than one commits when they are being closed. With this change, we move the trimming logic to the ctor of InternalEngine so we won't trim anything if we are going to open a read-only engine.
5 participants
Today we always trim unsafe commits (whose max_seq_no >= global checkpoint) before starting a read-write or read-only engine. This is mandatory for read-write engines because they must start with the safe commit. This is also fine for read-only engines since most of the cases we should have exactly one commit after closing an index (trimming is a noop). However, this is not safe for following indices which might have more than one commits when they are being closed.
With this change, we won't trim anything if we are going to open a read-only engine.
I will work on a follow up to skip resync/rollback if an index is closed and another follow-up to enable the sanity check in ReadOnly for following indices.
Relates #33888