[Segment Replication] Ensure replica's store always contains the previous commit point. #2551

mch2 · 2022-03-22T00:23:59Z

Description

This change:

Updates the cleanup and validation steps on replicas after a replication event occurs to prevent deleting files that are still required by the latest commit point.
Computes and sends replicas a list of recently merged away files that are still referenced by the primary's latest commit point. This allows replicas to fetch these files if not present locally. This can happen when a replica falls multiple commit points behind or if it starts after a primary has already existed and is in this state.
Update initial recovery sequence of replicas to copy from primary before lighting up as active. This fixes bug where replicas could not be added after primary.
Prevent replicas from attempting to a force merge if segrep is enabled.

Issues Resolved

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

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.

opensearch-ci-bot · 2022-03-22T00:43:08Z

❌ Gradle Check failure 5dd980e1c68f1e97613fdeba73491e46b66f6ec0
Log 3649

Reports 3649

This change: 1. Updates the cleanup and validation steps after a replication event occurs to prevent deleting files still referenced by both the on disk segments_N file and the in memory SegmentInfos. 2. Sends metadata diff of on disk segments with each copy event. This allows replicas that are multiple commit points behind to catch up. 3. Update initial recovery in IndexShard to copy segments before lighting up as active. This fixes bug where replicas could not be added after primary. Signed-off-by: Marc Handalian <[email protected]>

mch2 · 2022-03-22T18:00:34Z

force push is a rebase from the feature branch.

opensearch-ci-bot · 2022-03-22T18:16:56Z

❌ Gradle Check failure d51a440
Log 3671

Reports 3671

...er/src/internalClusterTest/java/org/opensearch/indices/replication/SegmentReplicationIT.java

Poojita-Raj · 2022-03-22T21:38:04Z

server/src/main/java/org/opensearch/index/shard/IndexShard.java

            segmentReplicationReplicaService.startReplication(
                checkpoint,
                this,
                source,
                new SegmentReplicationReplicaService.SegmentReplicationListener() {
                    @Override
                    public void onReplicationDone(SegmentReplicationState state) {
-                        markReplicationComplete();


Why would we remove this when we continue to use MarkAsReplicating()?

I moved this to when ReplicationTarget's finalize method completes because I didn't like SegmentReplicationListener being responsible for marking it as complete. With that said I think we can do better by moving it to the onDone/onCancel/onFail methods of SegmentReplicationTarget. Will also move markAsReplicating to ReplicationTarget as well so these classes don't have to manage that.

cleaned this up in latest commit, what do you think?

Wasn't the plan to move markAsReplicating to ReplicationTarget as well? Right now, that seems to be in ReplicaService so the lifecycle management is distributed across two classes, which smells. How do we improve this?

Hmm, maybe we can completely remove this state and make a synchronized check if ReplicationCollection has an ongoing replication for the shard. Would like to take this as a follow up change if thats ok.

Could you create a task to track it, so we don't forget?

server/src/main/java/org/opensearch/index/store/Store.java

server/src/main/java/org/opensearch/indices/replication/SegmentReplicationReplicaService.java

server/src/main/java/org/opensearch/indices/replication/copy/CopyState.java

Signed-off-by: Marc Handalian <[email protected]>

opensearch-ci-bot · 2022-03-23T00:16:44Z

❌ Gradle Check failure 7d5ab35
Log 3684

Reports 3684

server/src/main/java/org/opensearch/index/shard/IndexShard.java

server/src/main/java/org/opensearch/index/store/Store.java

...main/java/org/opensearch/indices/replication/checkpoint/TransportCheckpointInfoResponse.java

.../src/main/java/org/opensearch/indices/replication/copy/SegmentReplicationPrimaryService.java

kartg · 2022-03-23T18:27:53Z

.../src/main/java/org/opensearch/indices/replication/copy/SegmentReplicationPrimaryService.java

+                assert Transports.assertNotTransportThread(this + "[onFailure]");
+                logger.error("Failure", e);


Why this change in behavior?

Also, better log message?

The existing responseListener's onFailure was getting triggered but doing nothing. This at least logs the error on the primary. I've updated to also send the exception back to the replica.

server/src/main/java/org/opensearch/indices/replication/copy/SegmentReplicationTarget.java

...er/src/internalClusterTest/java/org/opensearch/indices/replication/SegmentReplicationIT.java

kartg · 2022-03-23T18:46:22Z

Outside of my review comments and the changes in this PR specifically, I wanted to put a high-level question - should we make the seg-rep tests more rigorous to validate the file structure on replica nodes to make sure they are consistent with the primary?

Our current asserts are around hit count, which seems largely sufficient given that our test case is checking if newly indexed documents show up on the replica. But it doesn't validate that the shards are point-in-time consistent

mch2 · 2022-03-24T20:48:25Z

Outside of my review comments and the changes in this PR specifically, I wanted to put a high-level question - should we make the seg-rep tests more rigorous to validate the file structure on replica nodes to make sure they are consistent with the primary?

Our current asserts are around hit count, which seems largely sufficient given that our test case is checking if newly indexed documents show up on the replica. But it doesn't validate that the shards are point-in-time consistent

Good call. I think we can do a decent comparison with the results of _cat/segments.

opensearch-ci-bot · 2022-03-24T20:54:10Z

❌ Gradle Check failure 8ce2c1a9c78301f9208b5c6a028044d229ed41b2
Log 3740

Reports 3740

opensearch-ci-bot · 2022-03-24T22:06:00Z

❌ Gradle Check failure c7791601588393a1d25388acc286985a824e3281
Log 3743

Reports 3743

.../src/main/java/org/opensearch/indices/replication/copy/SegmentReplicationPrimaryService.java

- Updated TrackShardRequestHandler to send error case back to replicas. - Renamed additionalFiles to pendingDeleteFiles in TransportCheckpointInfoResponse. - Refactored Store.cleanupAndVerify methods to remove duplication. Signed-off-by: Marc Handalian <[email protected]>

opensearch-ci-bot · 2022-03-25T01:14:48Z

❌ Gradle Check failure f0410ac5b3b8c13bba3ffbe240241ccc6d3bcf69
Log 3749

Reports 3749

opensearch-ci-bot · 2022-03-25T01:14:48Z

❌ Gradle Check failure 169cfc4
Log 3750

Reports 3750

mch2 requested a review from a team as a code owner March 22, 2022 00:24

mch2 changed the title ~~Ensure replica's store always contains the previous commit point.~~ [Segment Replication] Ensure replica's store always contains the previous commit point. Mar 22, 2022

mch2 assigned kartg and Poojita-Raj Mar 22, 2022

dblock requested review from kartg and Poojita-Raj March 22, 2022 14:09

mch2 force-pushed the deletefix branch from 5dd980e to d51a440 Compare March 22, 2022 17:58

mch2 unassigned Poojita-Raj and kartg Mar 22, 2022

Poojita-Raj approved these changes Mar 22, 2022

View reviewed changes

Chagnes from PR feedback.

7d5ab35

Signed-off-by: Marc Handalian <[email protected]>

kartg reviewed Mar 23, 2022

View reviewed changes

mch2 force-pushed the deletefix branch from 8ce2c1a to c779160 Compare March 24, 2022 21:45

kartg reviewed Mar 25, 2022

View reviewed changes

.../src/main/java/org/opensearch/indices/replication/copy/SegmentReplicationPrimaryService.java Outdated Show resolved Hide resolved

kartg approved these changes Mar 25, 2022

View reviewed changes

mch2 force-pushed the deletefix branch from c779160 to f0410ac Compare March 25, 2022 00:35

mch2 force-pushed the deletefix branch from f0410ac to 169cfc4 Compare March 25, 2022 00:42

mch2 merged commit 9bcee79 into opensearch-project:feature/segment-replication Mar 25, 2022

Poojita-Raj mentioned this pull request Mar 30, 2022

[BUG][Segment Replication] Gradle check failures #2414

Closed

kartg mentioned this pull request Mar 30, 2022

[META] Segment Replication Issue list #2194

Closed

31 tasks

mch2 deleted the deletefix branch August 16, 2022 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Segment Replication] Ensure replica's store always contains the previous commit point. #2551

[Segment Replication] Ensure replica's store always contains the previous commit point. #2551

mch2 commented Mar 22, 2022

opensearch-ci-bot commented Mar 22, 2022

mch2 commented Mar 22, 2022

opensearch-ci-bot commented Mar 22, 2022

Poojita-Raj Mar 22, 2022

mch2 Mar 22, 2022

mch2 Mar 22, 2022

kartg Mar 23, 2022

mch2 Mar 23, 2022

kartg Mar 25, 2022

opensearch-ci-bot commented Mar 23, 2022

kartg Mar 23, 2022

mch2 Mar 24, 2022

kartg commented Mar 23, 2022

mch2 commented Mar 24, 2022

opensearch-ci-bot commented Mar 24, 2022

opensearch-ci-bot commented Mar 24, 2022

opensearch-ci-bot commented Mar 25, 2022

opensearch-ci-bot commented Mar 25, 2022

		assert Transports.assertNotTransportThread(this + "[onFailure]");
		logger.error("Failure", e);

[Segment Replication] Ensure replica's store always contains the previous commit point. #2551

[Segment Replication] Ensure replica's store always contains the previous commit point. #2551

Conversation

mch2 commented Mar 22, 2022

Description

Issues Resolved

Check List

opensearch-ci-bot commented Mar 22, 2022

mch2 commented Mar 22, 2022

opensearch-ci-bot commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Mar 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartg commented Mar 23, 2022

mch2 commented Mar 24, 2022

opensearch-ci-bot commented Mar 24, 2022

opensearch-ci-bot commented Mar 24, 2022

opensearch-ci-bot commented Mar 25, 2022

opensearch-ci-bot commented Mar 25, 2022