Use ReplicationFailedException instead of OpensearchException in Repl… #4725

ayushKataria · 2022-10-10T13:13:40Z

…icationTarget

Signed-off-by: Ayush Kataria ayushkataria97@gmail.com

Description

Uses a narrower ReplicationFailedException instead of OpensearchException in ReplicationTarget and ReplicationListener

Issues Resolved

[List any issues this PR will resolve]

#4291

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
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

…icationTarget Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com>

Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com>

github-actions · 2022-10-10T14:44:34Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4252/
CommitID: f736a78

github-actions · 2022-10-10T14:49:47Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4253/
CommitID: 2d27461

Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com>

github-actions · 2022-10-10T15:13:01Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4262/
CommitID: a06998e

Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com>

github-actions · 2022-10-10T15:25:26Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4263/
CommitID: 92d09b6

Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com>

github-actions · 2022-10-10T17:24:51Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4269/
CommitID: 74830ff

mch2

Thanks for the change @ayushKataria. Some minor comments that I think are causing the test failures.

mch2 · 2022-10-10T22:40:31Z

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

-                new CancellableThreads.ExecutionCancelledException("replication was canceled reason [" + reason + "]");
+            final ReplicationFailedException executionCancelledException = new ReplicationFailedException(
+                "replication was canceled reason [" + reason + "]"
+            );
            notifyListener(executionCancelledException, false);


With notifyListener now accepting a ReplicationFailedException, we should wrap the cancellationException in ReplicationFailedException.

cancellableThreads.setOnCancel((reason, beforeCancelEx) -> { // This method only executes when cancellation is triggered by this node and caught by a call to checkForCancel, // SegmentReplicationSource does not share CancellableThreads. final CancellableThreads.ExecutionCancelledException executionCancelledException = new CancellableThreads.ExecutionCancelledException("replication was canceled reason [" + reason + "]"); notifyListener(new ReplicationFailedException("Segment replication failed to complete", executionCancelledException), false); throw executionCancelledException; });

There is some special logic above in SegmentReplicationTarget that overrides notifyListener to unwrap the failure and mark it as cancelled. This block will never trigger if the cancellation initiates from the primary however. So thats why the special logic is not in this setOnCancel block.

ahh, thanks for the help. For some reason I was not able to figure the trigger out exactly

mch2 · 2022-10-10T22:43:00Z

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

        // Cancellations still are passed to our SegmentReplicationListner as failures, if we have failed because of cancellation
        // update the stage.
        final Throwable cancelledException = ExceptionsHelper.unwrap(e, CancellableThreads.ExecutionCancelledException.class);
        if (cancelledException != null) {
            state.setStage(SegmentReplicationState.Stage.CANCELLED);
-            listener.onFailure(state(), (CancellableThreads.ExecutionCancelledException) cancelledException, sendShardFailure);
+            listener.onFailure(state(), new ReplicationFailedException(indexShard, cancelledException), sendShardFailure);


A nit - I don't think we need to cast or re-wrap the exception here.

@Override public void notifyListener(ReplicationFailedException e, boolean sendShardFailure) { // Cancellations still are passed to our SegmentReplicationListner as failures, if we have failed because of cancellation // update the stage. if (ExceptionsHelper.unwrap(e, CancellableThreads.ExecutionCancelledException.class) != null) { state.setStage(SegmentReplicationState.Stage.CANCELLED); } listener.onFailure(state(), e, sendShardFailure); }

mch2 · 2022-10-10T22:45:21Z

server/src/main/java/org/opensearch/indices/recovery/RecoveryTarget.java

+     * @param e                exception that encapsulates the failure
+     * @param sendShardFailure indicates whether to notify the master of the shard failure
+     */
+    public void fail(RecoveryFailedException e, boolean sendShardFailure) {


With RecoveryFailedException extending ReplicationFailedException I don't think we need to override here in RecoveryTarget?

Yeah, makes sense. This is redundant now, so will remove it.

Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com>

github-actions · 2022-10-11T04:58:03Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4349/
CommitID: c9bfc98

codecov-commenter · 2022-10-11T05:00:22Z

Codecov Report

Merging #4725 (f6c25fe) into main (d15795a) will decrease coverage by 0.00%.
The diff coverage is 46.69%.

@@             Coverage Diff              @@
##               main    #4725      +/-   ##
============================================
- Coverage     70.66%   70.65%   -0.01%     
- Complexity    57578    57594      +16     
============================================
  Files          4661     4669       +8     
  Lines        276662   276870     +208     
  Branches      40325    40346      +21     
============================================
+ Hits         195501   195624     +123     
- Misses        64926    64949      +23     
- Partials      16235    16297      +62

Impacted Files	Coverage Δ
.../delete/DeleteDecommissionStateRequestBuilder.java	`0.00% <0.00%> (ø)`
...apshots/restore/RestoreSnapshotRequestBuilder.java	`15.78% <0.00%> (-0.88%)`	⬇️
.../org/opensearch/client/support/AbstractClient.java	`32.61% <0.00%> (-0.32%)`	⬇️
.../java/org/opensearch/common/util/FeatureFlags.java	`50.00% <ø> (ø)`
...ateway/TransportNodesListGatewayStartedShards.java	`51.00% <0.00%> (-0.68%)`	⬇️
...h/index/shard/RemoveCorruptedShardDataCommand.java	`81.51% <ø> (ø)`
...java/org/opensearch/index/shard/StoreRecovery.java	`67.88% <ø> (+0.01%)`	⬆️
...h/index/store/InMemoryRemoteSnapshotDirectory.java	`0.00% <0.00%> (ø)`
...in/java/org/opensearch/indices/IndicesService.java	`69.65% <0.00%> (-0.25%)`	⬇️
...arch/indices/recovery/RecoveryFailedException.java	`100.00% <ø> (ø)`
... and 514 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

github-actions · 2022-10-11T05:05:08Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4350/
CommitID: c3e49c0

mch2

LGTM! Thanks for the change @ayushKataria!

github-actions · 2022-10-13T03:43:43Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4550/
CommitID: 1b1534b

github-actions · 2022-10-13T14:38:11Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4587/
CommitID: f6c25fe

github-actions · 2022-10-13T22:27:56Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4643/
CommitID: f6c25fe

opensearch-project#4725) * Use ReplicationFailedException instead of OpensearchException in ReplicationTarget Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * CHANGELOG.md updated Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * test fixes Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * spotless fix Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * spotless fix Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * fixes for failing test as suggested in PR comments Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com>

opensearch-trigger-bot · 2023-01-20T18:23:43Z

The backport to 2.x failed:

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

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
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-4725-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 6bae823cefad7373a748ce833ce75d80f48b380b
# Push it to GitHub
git push --set-upstream origin backport/backport-4725-to-2.x
# Go back to the original working tree
popd
# 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-4725-to-2.x.

opensearch-project#4725) * Use ReplicationFailedException instead of OpensearchException in ReplicationTarget Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * CHANGELOG.md updated Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * test fixes Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * spotless fix Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * spotless fix Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * fixes for failing test as suggested in PR comments Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com>

opensearch-project#4725) * Use ReplicationFailedException instead of OpensearchException in ReplicationTarget Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * CHANGELOG.md updated Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * test fixes Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * spotless fix Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * spotless fix Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * fixes for failing test as suggested in PR comments Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> Signed-off-by: Suraj Singh <surajrider@gmail.com>

…icationTarget (#5955) * Use ReplicationFailedException instead of OpensearchException in Repl… (#4725) * Use ReplicationFailedException instead of OpensearchException in ReplicationTarget Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * CHANGELOG.md updated Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * test fixes Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * spotless fix Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * spotless fix Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * fixes for failing test as suggested in PR comments Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> Signed-off-by: Suraj Singh <surajrider@gmail.com> * Update SegmentReplicationListener to use ReplicationFailedException Signed-off-by: Suraj Singh <surajrider@gmail.com> * Spotless fix Signed-off-by: Suraj Singh <surajrider@gmail.com> Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> Signed-off-by: Suraj Singh <surajrider@gmail.com> Co-authored-by: Ayush Kataria <31301636+ayushKataria@users.noreply.github.com>

…st backport Signed-off-by: Suraj Singh <surajrider@gmail.com>

Signed-off-by: Suraj Singh <surajrider@gmail.com> Signed-off-by: Suraj Singh <surajrider@gmail.com>

…icationTarget (#5955) * Use ReplicationFailedException instead of OpensearchException in Repl… (#4725) * Use ReplicationFailedException instead of OpensearchException in ReplicationTarget Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * CHANGELOG.md updated Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * test fixes Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * spotless fix Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * spotless fix Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> * fixes for failing test as suggested in PR comments Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> Signed-off-by: Suraj Singh <surajrider@gmail.com> * Update SegmentReplicationListener to use ReplicationFailedException Signed-off-by: Suraj Singh <surajrider@gmail.com> * Spotless fix Signed-off-by: Suraj Singh <surajrider@gmail.com> Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com> Signed-off-by: Suraj Singh <surajrider@gmail.com> Co-authored-by: Ayush Kataria <31301636+ayushKataria@users.noreply.github.com>

Use ReplicationFailedException instead of OpensearchException in Repl…

f736a78

…icationTarget Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com>

ayushKataria requested review from a team and reta as code owners October 10, 2022 13:13

CHANGELOG.md updated

2d27461

Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com>

test fixes

a06998e

Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com>

spotless fix

92d09b6

Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com>

spotless fix

74830ff

Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com>

saratvemulapalli requested a review from mch2 October 10, 2022 18:26

mch2 reviewed Oct 10, 2022

View reviewed changes

ayushKataria and others added 2 commits October 11, 2022 10:02

fixes for failing test as suggested in PR comments

c9bfc98

Signed-off-by: Ayush Kataria <ayushkataria97@gmail.com>

Merge branch 'main' into replicationexception

c3e49c0

ayushKataria requested a review from mch2 October 11, 2022 14:30

mch2 approved these changes Oct 12, 2022

View reviewed changes

Merge branch 'main' into replicationexception

1b1534b

Merge branch 'opensearch-project:main' into replicationexception

f6c25fe

mch2 merged commit 6bae823 into opensearch-project:main Oct 13, 2022

dreamer-89 added the backport 2.x Backport to 2.x branch label Jan 20, 2023

dreamer-89 mentioned this pull request Jan 20, 2023

Use ReplicationFailedException instead of OpensearchException in ReplicationTarget #5955

Merged

dreamer-89 added a commit to dreamer-89/OpenSearch that referenced this pull request Jan 20, 2023

Moves PR opensearch-project#4725 changelog entry to unreleased 2.x po…

0cac1a3

…st backport Signed-off-by: Suraj Singh <surajrider@gmail.com>

dreamer-89 mentioned this pull request Jan 20, 2023

Moves PR #4725 changelog entry to unreleased 2.x post backport #5960

Merged

6 tasks

kotwanikunal pushed a commit that referenced this pull request Jan 20, 2023

Moves PR #4725 changelog entry to unreleased 2.x post backport (#5960)

1a2387f

Signed-off-by: Suraj Singh <surajrider@gmail.com> Signed-off-by: Suraj Singh <surajrider@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ReplicationFailedException instead of OpensearchException in Repl… #4725

Use ReplicationFailedException instead of OpensearchException in Repl… #4725

ayushKataria commented Oct 10, 2022 •

edited

github-actions bot commented Oct 10, 2022

github-actions bot commented Oct 10, 2022

github-actions bot commented Oct 10, 2022

github-actions bot commented Oct 10, 2022

github-actions bot commented Oct 10, 2022

mch2 left a comment

mch2 Oct 10, 2022

ayushKataria Oct 11, 2022

mch2 Oct 10, 2022

mch2 Oct 10, 2022

ayushKataria Oct 11, 2022

github-actions bot commented Oct 11, 2022

codecov-commenter commented Oct 11, 2022 •

edited

github-actions bot commented Oct 11, 2022

mch2 left a comment

github-actions bot commented Oct 13, 2022

github-actions bot commented Oct 13, 2022

github-actions bot commented Oct 13, 2022

opensearch-trigger-bot bot commented Jan 20, 2023

Use ReplicationFailedException instead of OpensearchException in Repl… #4725

Use ReplicationFailedException instead of OpensearchException in Repl… #4725

Conversation

ayushKataria commented Oct 10, 2022 • edited

Description

Issues Resolved

Check List

github-actions bot commented Oct 10, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 10, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 10, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 10, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 10, 2022

Gradle Check (Jenkins) Run Completed with:

mch2 left a comment

Choose a reason for hiding this comment

mch2 Oct 10, 2022

Choose a reason for hiding this comment

ayushKataria Oct 11, 2022

Choose a reason for hiding this comment

mch2 Oct 10, 2022

Choose a reason for hiding this comment

mch2 Oct 10, 2022

Choose a reason for hiding this comment

ayushKataria Oct 11, 2022

Choose a reason for hiding this comment

github-actions bot commented Oct 11, 2022

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Oct 11, 2022 • edited

Codecov Report

github-actions bot commented Oct 11, 2022

Gradle Check (Jenkins) Run Completed with:

mch2 left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 13, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 13, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 13, 2022

Gradle Check (Jenkins) Run Completed with:

opensearch-trigger-bot bot commented Jan 20, 2023

ayushKataria commented Oct 10, 2022 •

edited

codecov-commenter commented Oct 11, 2022 •

edited