Replay history of operations in remote recovery #39153
Conversation
dnhatn
added
blocker
v7.0.0
:Distributed Indexing/CCR
dnhatn
requested review from
martijnvg,
Tim-Brooks,
bleskes,
ywelsch,
jasontedor and
DaveCTurner
|
Pinging @elastic/es-distributed |
|
run elasticsearch-ci/default-distro |
There was a problem hiding this comment.
Thanks @dnhatn. I've had an initial look and this looks already great. I'm not super happy with restoreShard returning a Translog.Snapshot, as it's now up to the caller to complete building the shard but also see that it's tricky to otherwise keep information about session etc. local to the repository implementation.
server/src/main/java/org/elasticsearch/index/shard/StoreRecovery.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/shard/StoreRecovery.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/shard/StoreRecovery.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/shard/StoreRecovery.java
Outdated
Show resolved
Hide resolved
| * to the max_seq_no. Then we won't have a safe commit for the restoring commit is not safe (missing translog). | ||
| * To maintain the safe commit assumption, we have to forcefully flush a new commit here. | ||
| */ | ||
| indexShard.flush(new FlushRequest().force(true).waitIfOngoing(true)); |
There was a problem hiding this comment.
should we make sure that the global checkpoint is up-to-date (i.e. >= max-seq-no in the new commit that we create here) before calling this? Otherwise the shard will be marked as in-sync in the cluster state while the commit here will only become safe commit when the shard is locally started (and the gcp advanced). The main property we're after here is that every in-sync shard copy has a safe commit, which (AFAICS) is not guaranteed by the current recovery logic.
| */ | ||
| public static void assertSafeCommitExists(IndexShard shard) throws IOException { | ||
| try { | ||
| if (shard.state != IndexShardState.STARTED) { |
There was a problem hiding this comment.
a shard in POST_RECOVERY should also satisfy this property already, see my comment above
| "checkpoint=" + newCommitInfo.localCheckpoint + " max_seq_no=" + newCommitInfo.maxSeqNo); | ||
| } | ||
| } | ||
| } | ||
| indexShard.finalizeRecovery(); | ||
| indexShard.postRecovery("restore done"); |
There was a problem hiding this comment.
should we assert in post_recovery that we have a safe commit?
There was a problem hiding this comment.
I added an assertion here, but this assertion may slow down our tests. I will post the difference.
There was a problem hiding this comment.
Sadly, we don't have this invariant with closed indices because the global checkpoint is not persisted to the translog checkpoint during recovery.
|
Discussed with Yannick on another channel, I marked this as WIP since we need to make broader (maybe unrelated) changes to this PR to achieve the safe commit invariant. |
# Conflicts: # server/src/test/java/org/elasticsearch/index/replication/IndexLevelReplicationTests.java
|
We now can achieve the safe commit invariant. I will open smaller pull requests for unrelated changes. |
|
@ywelsch This is ready for another round. Can you please give it another go. Thank you! |
|
I am closing this PR as the safe commit invariant does not hold for follower indices. |
The safe commit invariant does not hold for the following indices because we do not replay the history in remote recovery.
Relates #39000
Relates #35975