Skip to content
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] Wire up segment replication with peer recovery and add ITs. #3743

Merged
merged 6 commits into from
Jul 22, 2022

Conversation

mch2
Copy link
Member

@mch2 mch2 commented Jun 29, 2022

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

Description

This change wires up segment replication under the feature flag and adds some ITs. This PR reuses peer recovery with segment replication. Replicas will be in sync in that they have durably persisted all docs, but will not have all docs searchable until the next refresh on the primary & replication event.

Other changes:

  • Add null check when computing max segment version in store.java.
    With segment replication enabled Lucene does not set the SegmentInfos
    min segment version on a refresh or flush, leaving the default value as null. The value is only set when reading infos from a commit point.
  • 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.

Issues Resolved

#3483

Check List

  • [ x] New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • [ x] 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.

@peterzhuamazon
Copy link
Member

Hi @mch2 please rebase on main to consume latest gradle check changes.
Thanks.

@github-actions

This comment was marked as outdated.

@mch2
Copy link
Member Author

mch2 commented Jun 30, 2022

FYI this is still a draft, I expect the gradle check to fail here as I'm hardcoding the segrep feature flag to true for now.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

final String translogUUID = store.getMetadata().getCommitUserData().get(TRANSLOG_UUID_KEY);
Translog.createEmptyTranslog(
indexShard.shardPath().resolveTranslog(),
indexShard.shardId(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 nitpicks here:

First, remove the shardId member variable in this class and have shardId() return indexShard.shardId() instead. Then use shardId() here to parallel the else clause

Second, (and you can punt this to a follow-up PR), it'd be nice to have the arguments of createEmptyTranslog maintain a consistent ordering across the various overloaded definitions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 - Done. 2 - Will follow up with this to not inflate this pr.

mch2 added 4 commits July 21, 2022 12:56
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 <[email protected]>
…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 <[email protected]>
This PR wires up segment replication and adds some initial integration tests.

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

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

…d and SegmentInfos can be built.

Fix nitpicks in PR feedback.

Signed-off-by: Marc Handalian <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@mch2 mch2 merged commit 197909c into opensearch-project:main Jul 22, 2022
@mch2 mch2 deleted the wireAfterCopy branch July 22, 2022 16:31
kartg added a commit to kartg/OpenSearch that referenced this pull request Aug 8, 2022
The original change included a javadoc update to the IT class, but the test class was never backported from opensearch-project#3743 to 2.x. Since the scope of this change is to backport dependency upgrades only, this class has been removed.

Signed-off-by: Kartik Ganesh <[email protected]>
@kartg kartg mentioned this pull request Aug 8, 2022
5 tasks
kartg added a commit to kartg/OpenSearch that referenced this pull request Aug 8, 2022
The original change included a javadoc update to the IT class, but the test class was never backported from opensearch-project#3743 to 2.x. Since the scope of this change is to backport dependency upgrades only, this class has been removed.

Signed-off-by: Kartik Ganesh <[email protected]>
(cherry picked from commit d6ead92)
@kartg kartg mentioned this pull request Aug 8, 2022
5 tasks
kartg added a commit that referenced this pull request Aug 8, 2022
* Upgrade dependencies (#4047)

* Upgrading dependencies for hadoop and aws-java-sdk

Signed-off-by: Vacha Shah <[email protected]>

* Fixing precommit

Signed-off-by: Vacha Shah <[email protected]>

* Upgrading transitive dependencies

Signed-off-by: Vacha Shah <[email protected]>

* Excluding transitive dependencies

Signed-off-by: Vacha Shah <[email protected]>
(cherry picked from commit 88f5537)

* Removing seg-rep IT class from the scope of this backport

The original change included a javadoc update to the IT class, but the test class was never backported from #3743 to 2.x. Since the scope of this change is to backport dependency upgrades only, this class has been removed.

Signed-off-by: Kartik Ganesh <[email protected]>

Co-authored-by: Vacha Shah <[email protected]>
kartg added a commit that referenced this pull request Aug 8, 2022
* Upgrade dependencies (#4047)

* Upgrading dependencies for hadoop and aws-java-sdk

Signed-off-by: Vacha Shah <[email protected]>

* Fixing precommit

Signed-off-by: Vacha Shah <[email protected]>

* Upgrading transitive dependencies

Signed-off-by: Vacha Shah <[email protected]>

* Excluding transitive dependencies

Signed-off-by: Vacha Shah <[email protected]>
(cherry picked from commit 88f5537)
(cherry picked from commit 7d92565)

* Removing seg-rep IT class from the scope of this backport

The original change included a javadoc update to the IT class, but the test class was never backported from #3743 to 2.x. Since the scope of this change is to backport dependency upgrades only, this class has been removed.

Signed-off-by: Kartik Ganesh <[email protected]>
(cherry picked from commit d6ead92)

Co-authored-by: Vacha Shah <[email protected]>
@kartg kartg added the backport 2.x Backport to 2.x branch label Aug 8, 2022
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-3743-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 197909c71f91f1e26da7bc6c7218b53fbb73cdae
# Push it to GitHub
git push --set-upstream origin backport/backport-3743-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3743-to-2.x.

Rishikesh1159 pushed a commit to Rishikesh1159/OpenSearch that referenced this pull request Aug 10, 2022
…and add ITs. (opensearch-project#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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* Add test to ensure replicas use primary translog uuid with segrep.

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

* Update SegmentReplicationIT to assert previous commit points are valid and SegmentInfos can be built.
Fix nitpicks in PR feedback.

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

* Fix test with Assert.fail to include a message.

Signed-off-by: Marc Handalian <[email protected]>
Rishikesh1159 pushed a commit to Rishikesh1159/OpenSearch that referenced this pull request Aug 17, 2022
…and add ITs. (opensearch-project#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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* Add test to ensure replicas use primary translog uuid with segrep.

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

* Update SegmentReplicationIT to assert previous commit points are valid and SegmentInfos can be built.
Fix nitpicks in PR feedback.

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

* Fix test with Assert.fail to include a message.

Signed-off-by: Marc Handalian <[email protected]>
Rishikesh1159 added a commit that referenced this pull request Aug 17, 2022
…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 <[email protected]>

* [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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* Add test to ensure replicas use primary translog uuid with segrep.

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

* Update SegmentReplicationIT to assert previous commit points are valid and SegmentInfos can be built.
Fix nitpicks in PR feedback.

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

* Fix test with Assert.fail to include a message.

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

* 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 <[email protected]>

* Fix isAheadOf logic for ReplicationCheckpoint comparison (#4112)

Signed-off-by: Suraj Singh <[email protected]>

* Fix waitUntil refresh policy for segrep enabled indices. (#4137)

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

* Add IndexShard#getLatestReplicationCheckpoint behind segrep enable feature flag (#4163)

* Add IndexShard#getLatestReplicationCheckpoint behind segrep enable feature flag

Signed-off-by: Suraj Singh <[email protected]>

* Address review comment. Move tests to SegmentReplicationIndexShardTests

Signed-off-by: Suraj Singh <[email protected]>

* Add segrep enbaled index settings in TargetServiceTests, SourceHandlerTests

Signed-off-by: Suraj Singh <[email protected]>

* [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 <[email protected]>

* Clean up cancel methods.

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

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

* [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 <[email protected]>

Signed-off-by: Poojita Raj <[email protected]>

* Update recoveryDiff logic to ingore missing files causing exception on replica during copy

Signed-off-by: Suraj Singh <[email protected]>

* Address review comments

Signed-off-by: Suraj Singh <[email protected]>

Signed-off-by: Poojita Raj <[email protected]>
Signed-off-by: Suraj Singh <[email protected]>
Co-authored-by: Poojita Raj <[email protected]>

* [Segment Replication] Adding PrimaryMode check before publishing checkpoint and processing a received checkpoint. (#4157)

* Adding PrimaryMode check before publishing checkpoint.

Signed-off-by: Rishikesh1159 <[email protected]>

* Applying spotless check

Signed-off-by: Rishikesh1159 <[email protected]>

* Moving segrep specific tests to SegmentReplicationIndexShardTests.

Signed-off-by: Rishikesh1159 <[email protected]>

* Adding logic and tests for rejecting checkpoints if shard is in PrimaryMode.

Signed-off-by: Rishikesh1159 <[email protected]>

* Applying ./gradlew :server:spotlessApply.

Signed-off-by: Rishikesh1159 <[email protected]>

* Applying ./gradlew :server:spotlessApply

Signed-off-by: Rishikesh1159 <[email protected]>

* Changing log level to warn in shouldProcessCheckpoint() of IndexShard.java class.

Signed-off-by: Rishikesh1159 <[email protected]>

* Removing unnecessary lazy logging in shouldProcessCheckpoint().

Signed-off-by: Rishikesh1159 <[email protected]>

Signed-off-by: Rishikesh1159 <[email protected]>

* [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 <[email protected]>

* Address review comments, assertBusy on doc count rather than sleep

Signed-off-by: Suraj Singh <[email protected]>

Signed-off-by: Suraj Singh <[email protected]>

* 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 <[email protected]>

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

* 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 <[email protected]>

* Fix spotless.

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

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

Signed-off-by: Rishikesh1159 <[email protected]>
Signed-off-by: Marc Handalian <[email protected]>
Signed-off-by: Suraj Singh <[email protected]>
Signed-off-by: Poojita Raj <[email protected]>
Co-authored-by: Marc Handalian <[email protected]>
Co-authored-by: Suraj Singh <[email protected]>
Co-authored-by: Poojita Raj <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants