-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Segment Replication] Checkpoint Replay on Replica Shard #3658
[Segment Replication] Checkpoint Replay on Replica Shard #3658
Conversation
… tests Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
start gradle check |
server/src/main/java/org/opensearch/indices/replication/SegmentReplicationTargetService.java
Show resolved
Hide resolved
server/src/main/java/org/opensearch/indices/replication/SegmentReplicationTargetService.java
Outdated
Show resolved
Hide resolved
// if we received a checkpoint during the copy event that is ahead of this | ||
// try and process it. | ||
if (getLatestReceivedCheckpoint(replicaShard.shardId()).isAheadOf(replicaShard.getLatestReplicationCheckpoint())) { | ||
onNewCheckpoint(getLatestReceivedCheckpoint(replicaShard.shardId()), replicaShard); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little concerned that this recursive call could lead to a long-running thread and/or function stack overflow. As a follow-up to this PR, we should look into wrapping checkpoint processing in a Runnable
so we can execute it async from the new-checkpoint notification.
cc @mch2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason not to do this now? @Rishikesh1159 @kartg ? We had a similar implementation in the feature branch - https://github.com/opensearch-project/OpenSearch/blob/feature/segment-replication/server/src/main/java/org/opensearch/index/shard/IndexShard.java#L3683
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, i didn't realize the POC had the threading in place. Yes, if we have prior art, we can pull it into this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QQ: Do newer checkpoints supersede older checkpoints? What happens when the replication is slow is backlogged with too many checkpoints to process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I wrapped checkpoint processing in a Runnable in new commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bukhtawar we only store one latest received checkpoint and replace it whenever there is a new checkpoint. We don't store all received checkpoints to replica shard. So, even when replication is slow, we won't be backlogged with too many checkpoints, because we process only one checkpoint (which is latest received checkpoint)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks and I am assuming replication usually happens on the generic
threadpool to start with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rishikesh1159, @mch2 I would recommend doing explicit checks rather than simply forking to guarantee we don't end up using the same thread
void execute(Runnable runnable, Thread originalThread) {
if (originalThread == Thread.currentThread()) {
fork(runnable);
} else {
runnable.run();
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-resolved one of my earlier comments, and the others are nitpicks.
Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Rishikesh1159 left a comment on explicit checks on thread before we fork
@@ -177,6 +177,39 @@ public void testNewCheckpoint_validationPassesAndReplicationFails() throws IOExc | |||
closeShard(indexShard, false); | |||
} | |||
|
|||
public void testReplicationOnDone() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rishikesh1159 : Will be good to have test cases for failure scenario, multiple checkpoints and IT exercising this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have already covered failure scenario in previous PR.For multiple checkpoints we are already passing 2 checkpoints in many of our tests, so not sure if there is need to multiple checkpoints test case, But if needed we can do that also. And for IT for this, yes it is good to have an IT for this code, but I think we can make a different PR for that
Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
…project#3658) * Adding Latest Recevied checkpoint, replay checkpoint logic along with tests Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
…aining segment replication changes (#4243) * [Segment Replication] Checkpoint Replay on Replica Shard (#3658) * Adding Latest Recevied checkpoint, replay checkpoint logic along with tests Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * [Segment Replication] Wire up segment replication with peer recovery and add ITs. (#3743) * Add null check when computing max segment version. With segment replication enabled it is possible Lucene does not set the SegmentInfos min segment version, leaving the default value as null. Signed-off-by: Marc Handalian <handalm@amazon.com> * Update peer recovery to set the translogUUID of replicas to the UUID generated on the primary. This change updates the UUID when the translog is created to the value stored in the passed segment userdata. This is to ensure during failover scenarios that the replica can be promoted and not have a uuid mismatch with the value stored in user data. Signed-off-by: Marc Handalian <handalm@amazon.com> * Wire up Segment Replication under the feature flag. This PR wires up segment replication and adds some initial integration tests. Signed-off-by: Marc Handalian <handalm@amazon.com> * Add test to ensure replicas use primary translog uuid with segrep. Signed-off-by: Marc Handalian <handalm@amazon.com> * Update SegmentReplicationIT to assert previous commit points are valid and SegmentInfos can be built. Fix nitpicks in PR feedback. Signed-off-by: Marc Handalian <handalm@amazon.com> * Fix test with Assert.fail to include a message. Signed-off-by: Marc Handalian <handalm@amazon.com> * Disable shard idle with segment replication. (#4118) This change disables shard idle when segment replication is enabled. Primary shards will only push out new segments on refresh, we do not want to block this based on search behavior. Replicas will only refresh on an externally provided SegmentInfos, so we do not want search requests to hang waiting for a refresh. Signed-off-by: Marc Handalian <handalm@amazon.com> * Fix isAheadOf logic for ReplicationCheckpoint comparison (#4112) Signed-off-by: Suraj Singh <surajrider@gmail.com> * Fix waitUntil refresh policy for segrep enabled indices. (#4137) Signed-off-by: Marc Handalian <handalm@amazon.com> * Add IndexShard#getLatestReplicationCheckpoint behind segrep enable feature flag (#4163) * Add IndexShard#getLatestReplicationCheckpoint behind segrep enable feature flag Signed-off-by: Suraj Singh <surajrider@gmail.com> * Address review comment. Move tests to SegmentReplicationIndexShardTests Signed-off-by: Suraj Singh <surajrider@gmail.com> * Add segrep enbaled index settings in TargetServiceTests, SourceHandlerTests Signed-off-by: Suraj Singh <surajrider@gmail.com> * [Segment Replication] Fix OngoingSegmentReplications to key by allocation ID instead of DiscoveryNode. (#4182) * Fix OngoingSegmentReplications to key by allocation ID instead of DiscoveryNode. This change fixes segrep to work with multiple shards per node by keying ongoing replications on allocation ID. This also updates cancel methods to ensure state is properly cleared on shard cancel. Signed-off-by: Marc Handalian <handalm@amazon.com> * Clean up cancel methods. Signed-off-by: Marc Handalian <handalm@amazon.com> Signed-off-by: Marc Handalian <handalm@amazon.com> * [Bug] [Segment Replication] Update store metadata recovery diff logic to ignore missing files causing exception (#4185) * Update Store for segment replication dif Signed-off-by: Poojita Raj <poojiraj@amazon.com> Signed-off-by: Poojita Raj <poojiraj@amazon.com> * Update recoveryDiff logic to ingore missing files causing exception on replica during copy Signed-off-by: Suraj Singh <surajrider@gmail.com> * Address review comments Signed-off-by: Suraj Singh <surajrider@gmail.com> Signed-off-by: Poojita Raj <poojiraj@amazon.com> Signed-off-by: Suraj Singh <surajrider@gmail.com> Co-authored-by: Poojita Raj <poojiraj@amazon.com> * [Segment Replication] Adding PrimaryMode check before publishing checkpoint and processing a received checkpoint. (#4157) * Adding PrimaryMode check before publishing checkpoint. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Applying spotless check Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Moving segrep specific tests to SegmentReplicationIndexShardTests. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Adding logic and tests for rejecting checkpoints if shard is in PrimaryMode. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Applying ./gradlew :server:spotlessApply. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Applying ./gradlew :server:spotlessApply Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Changing log level to warn in shouldProcessCheckpoint() of IndexShard.java class. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Removing unnecessary lazy logging in shouldProcessCheckpoint(). Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * [Segment Replication] Wait for documents to replicate to replica shards (#4236) * [Segment Replication] Add thread sleep to account for replica lag in delete operations test Signed-off-by: Suraj Singh <surajrider@gmail.com> * Address review comments, assertBusy on doc count rather than sleep Signed-off-by: Suraj Singh <surajrider@gmail.com> Signed-off-by: Suraj Singh <surajrider@gmail.com> * Segment Replication - Remove unnecessary call to markAllocationIdAsInSync. (#4224) This PR Removes an unnecessary call to markAllocationIdAsInSync on the primary shard when replication events complete. Recovery will manage this initial call. Signed-off-by: Marc Handalian <handalm@amazon.com> Signed-off-by: Marc Handalian <handalm@amazon.com> * Segment Replication - Add additional unit tests for update & delete ops. (#4237) * Segment Replication - Add additional unit tests for update & delete operations. Signed-off-by: Marc Handalian <handalm@amazon.com> * Fix spotless. Signed-off-by: Marc Handalian <handalm@amazon.com> Signed-off-by: Marc Handalian <handalm@amazon.com> Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> Signed-off-by: Marc Handalian <handalm@amazon.com> Signed-off-by: Suraj Singh <surajrider@gmail.com> Signed-off-by: Poojita Raj <poojiraj@amazon.com> Co-authored-by: Marc Handalian <handalm@amazon.com> Co-authored-by: Suraj Singh <surajrider@gmail.com> Co-authored-by: Poojita Raj <poojiraj@amazon.com>
Description
This PR will allow us to store latest received checkpoint for replica shard and replay the latest checkpoint after previous checkpoint replication process is done.
This is a part of the process of merging our feature branch - feature/segment-replication - back into main by re-PRing our changes from the feature branch.
The breakdown of the plan to merge to main is detailed here: #2355
For added context on segment replication - here's the design proposal #2229
Issues Resolved
#3558
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.