-
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
Remove PRRL creation/deletion in peer recovery of remote store enabled replica #4954
Conversation
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
6f88c46
to
79c1d1a
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## main #4954 +/- ##
=========================================
Coverage 70.87% 70.88%
+ Complexity 58105 58071 -34
=========================================
Files 4708 4711 +3
Lines 277559 277604 +45
Branches 40189 40190 +1
=========================================
+ Hits 196729 196780 +51
+ Misses 64722 64690 -32
- Partials 16108 16134 +26
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Gradle Check (Jenkins) Run Completed with:
|
e6cd4a9
to
77d0d3d
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
@Bukhtawar @sachinpkale Can you take a look at this? |
Changes look good to me . Few clarifying questions :
|
@gbbafna thanks for the review -
There are units tests (OpenSearchTestCase) - ReplicaRecoveryWithRemoteTranslogOnPrimaryTests. Apart from that, validated the same by running the recovery with tweaking some changes locally. There would be more IT tests added once we have remote translog changes merged.
PRRL would still be used for doing primary-primary peer recovery. PRRL helps in doing sequence based recovery by replaying the same lucene operations on the recovering replica (which would be bumped to primary). |
@@ -324,6 +324,7 @@ void awaitEmpty() { | |||
|
|||
private final class ShardRecoveryContext { | |||
final Map<RecoverySourceHandler, RemoteRecoveryTargetHandler> recoveryHandlers = new HashMap<>(); | |||
private final RecoverySourceHandlerFactory recoverySourceHandlerFactory = new RecoverySourceHandlerFactory(); |
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.
nit: This can be a singleton.
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.
Java does not allow creating static fields within inner classes. It's supported, I guess, from 16/17+.
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.
Why is the factory an instance at all? It contains no state. I think you can make RecoverySourceHandlerFactory#create
static and call it below without creating an instance.
StartRecoveryRequest request, | ||
RecoverySettings recoverySettings | ||
) { | ||
boolean isReplicaRecoveryWithRemoteTranslog = request.isPrimaryRelocation() == false && shard.isRemoteTranslogEnabled(); |
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.
Why are we checking request.isPrimaryRelocation() == false
?
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.
request.isPrimaryRelocation() == false
is replica recovery. We want to use the RemoteStoreReplicaRecoverySourceHandler only for recovering the replicas. When request.isPrimaryRelocation() == true
, it is primary-primary recovery for which it would be DefaultRecoverySourceHandler. Primary-primary recovery would involve translogs and lucene operation replay during the peer recovery.
@@ -215,7 +215,7 @@ public void writeFileChunk( | |||
}); | |||
} | |||
}; | |||
RecoverySourceHandler handler = new RecoverySourceHandler( | |||
RecoverySourceHandler handler = new DefaultRecoverySourceHandler( |
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.
These tests essentially become tests for DefaultRecoverySourceHandler
. Don't we need tests for RemoteStoreReplicaRecoverySourceHandler
as well?
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.
The RecoverySourceHandlerTests
now covers both the RecoverySourceHandler and DefaultRecoverySourceHandler. The tests for RemoteStoreReplicaRecoverySourceHandler were written in an older PR and is present in ReplicaRecoveryWithRemoteTranslogOnPrimaryTests.java
. Let me decompose it and create another test class dedicated to the new Handler.
@@ -902,7 +902,7 @@ private boolean invariant() { | |||
if (primaryMode && indexSettings.isSoftDeleteEnabled() && hasAllPeerRecoveryRetentionLeases) { | |||
// all tracked shard copies have a corresponding peer-recovery retention lease | |||
for (final ShardRouting shardRouting : routingTable.assignedShards()) { | |||
if (checkpoints.get(shardRouting.allocationId().getId()).tracked) { | |||
if (checkpoints.get(shardRouting.allocationId().getId()).tracked && !indexSettings().isRemoteTranslogStoreEnabled()) { |
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.
This is required as currently if a shard is tracked then it is expected to have a corresponding retention lease.
561d668
to
5146fdc
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Ashish Singh <[email protected]>
Signed-off-by: Ashish Singh <[email protected]>
Signed-off-by: Ashish Singh <[email protected]>
Signed-off-by: Ashish Singh <[email protected]>
Gradle Check (Jenkins) Run Completed 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.
Looks good, mostly minor comments
@@ -106,21 +102,21 @@ | |||
* | |||
* @opensearch.internal | |||
*/ | |||
public class RecoverySourceHandler { | |||
public abstract class RecoverySourceHandler { |
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.
Can you make the constructor package-private to prevent this from being extended in ways not intended? I'm assuming this should only ever be instantiated via one of the subclasses in this package.
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.
Ack
import org.opensearch.index.shard.IndexShard; | ||
|
||
/** | ||
* Factory that supplies {@link RecoverySourceHandler}. |
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.
Please add @opensearch.internal
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.
Ack
import java.util.function.Consumer; | ||
|
||
/** | ||
* This handler is used when peer recovery target is a remote store enabled replica. |
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.
Please add @opensearch.internal
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.
Ack
|
||
/** | ||
* This handler is used for the peer recovery when there is no remote store available for segments/translogs. TODO - | ||
* Add more details as this is refactored further. |
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.
Please add @opensearch.internal
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.
Ack
@@ -324,6 +324,7 @@ void awaitEmpty() { | |||
|
|||
private final class ShardRecoveryContext { | |||
final Map<RecoverySourceHandler, RemoteRecoveryTargetHandler> recoveryHandlers = new HashMap<>(); | |||
private final RecoverySourceHandlerFactory recoverySourceHandlerFactory = new RecoverySourceHandlerFactory(); |
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.
Why is the factory an instance at all? It contains no state. I think you can make RecoverySourceHandlerFactory#create
static and call it below without creating an instance.
import java.util.function.Consumer; | ||
|
||
/** | ||
* This handler is used for the peer recovery when there is no remote store available for segments/translogs. TODO - |
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.
It is probably better to document this based on what it is/what it does, versus what it is not. e.g. "this handler is used for node-to-node recovery..."
It might also be better to give it a name more descriptive that "default". "Default" makes sense when you are adding a new alternative but will be less obvious over time. Maybe something like "RetentionLeaseRecoverySourceHandler" or "LocalRecoverySourceHandler"?
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.
Makes sense. Will make the change.
@Override | ||
protected void innerRecoveryToTarget(ActionListener<RecoveryResponse> listener, Consumer<Exception> onFailure) throws IOException { | ||
// A replica of an index with remote translog does not require the translogs locally and keeps receiving the | ||
// updated segments file on refresh, flushes, and merges. We plan to make the existing replication call to |
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.
The "We plan to" comment is confusing here. It suggests something here needs to change in the future. Are these sentences needed beyond the first sentence that says a local translog isn't needed?
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.
Yes, the "we plan to" comment tells us what is coming as this is an incremental PR to the bigger effort. However, I get that we can make it shorter and concise. I have reworded the "we plan to" section. Let me know if this makes sense.
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.
The update looks good, thanks!
I know this is nit-picky but comments like "we plan to..." very often become stale and will still be present in the code even after all the planned work is done, because the future work may not touch this exact piece of code again. So unless there is a strong need to document future intent I find it is better to try to avoid it.
Signed-off-by: Ashish Singh <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
@ashking94 Should this be backported to 2.x? I've added the label but want to get confirmation from you. |
The backport to
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-4954-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0be3afbd249f771057663059279b4d02e4d282df
# Push it to GitHub
git push --set-upstream origin backport/backport-4954-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 |
Yes, the plan is to get request level durability in 2.5 for which this is an incremental step. |
Thanks for the review, @andrross, @sachinpkale & @gbbafna! |
@ashking94 Auto-backport failed. Can you go ahead and create the backport PR? |
…d replica (opensearch-project#4954) * Add RecoverySourceHandlerFactory for extensibility Signed-off-by: Ashish Singh <[email protected]> * recoverToTarget made extensible to allow multiple implementations Signed-off-by: Ashish Singh <[email protected]> * Remove PRRL after SendFileStep in Peer Recovery Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Remove CHANGELOG entry as this is incremental PR Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> Signed-off-by: Ashish Singh <[email protected]>
…d replica (opensearch-project#4954) * Add RecoverySourceHandlerFactory for extensibility Signed-off-by: Ashish Singh <[email protected]> * recoverToTarget made extensible to allow multiple implementations Signed-off-by: Ashish Singh <[email protected]> * Remove PRRL after SendFileStep in Peer Recovery Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Remove CHANGELOG entry as this is incremental PR Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> Signed-off-by: Ashish Singh <[email protected]>
…5731) * Remove PRRL creation/deletion in peer recovery of remote store enabled replica (#4954) * Add RecoverySourceHandlerFactory for extensibility Signed-off-by: Ashish Singh <[email protected]> * recoverToTarget made extensible to allow multiple implementations Signed-off-by: Ashish Singh <[email protected]> * Remove PRRL after SendFileStep in Peer Recovery Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Remove CHANGELOG entry as this is incremental PR Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> Signed-off-by: Ashish Singh <[email protected]> * Enhance CheckpointState to support no-op replication (#5282) * CheckpointState enhanced to support no-op replication Signed-off-by: Ashish Singh <[email protected]> Co-authored-by: Bukhtawar Khan<[email protected]> Signed-off-by: Ashish Singh <[email protected]> * Add transport action for primary term validation for remote-backed indices (#5616) Add transport action for primary term validation for remote-backed indices Signed-off-by: Ashish Singh <[email protected]> Signed-off-by: Ashish Singh <[email protected]>
…pensearch-project#5731) * Remove PRRL creation/deletion in peer recovery of remote store enabled replica (opensearch-project#4954) * Add RecoverySourceHandlerFactory for extensibility Signed-off-by: Ashish Singh <[email protected]> * recoverToTarget made extensible to allow multiple implementations Signed-off-by: Ashish Singh <[email protected]> * Remove PRRL after SendFileStep in Peer Recovery Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Remove CHANGELOG entry as this is incremental PR Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> Signed-off-by: Ashish Singh <[email protected]> * Enhance CheckpointState to support no-op replication (opensearch-project#5282) * CheckpointState enhanced to support no-op replication Signed-off-by: Ashish Singh <[email protected]> Co-authored-by: Bukhtawar Khan<[email protected]> Signed-off-by: Ashish Singh <[email protected]> * Add transport action for primary term validation for remote-backed indices (opensearch-project#5616) Add transport action for primary term validation for remote-backed indices Signed-off-by: Ashish Singh <[email protected]> Signed-off-by: Ashish Singh <[email protected]>
* Remove PRRL creation/deletion in peer recovery of remote store enabled replica (#4954) * Add RecoverySourceHandlerFactory for extensibility Signed-off-by: Ashish Singh <[email protected]> * recoverToTarget made extensible to allow multiple implementations Signed-off-by: Ashish Singh <[email protected]> * Remove PRRL after SendFileStep in Peer Recovery Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Remove CHANGELOG entry as this is incremental PR Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> Signed-off-by: Ashish Singh <[email protected]> * Enhance CheckpointState to support no-op replication (#5282) * CheckpointState enhanced to support no-op replication Signed-off-by: Ashish Singh <[email protected]> Co-authored-by: Bukhtawar Khan<[email protected]> Signed-off-by: Ashish Singh <[email protected]> * Add transport action for primary term validation for remote-backed indices (#5616) Add transport action for primary term validation for remote-backed indices Signed-off-by: Ashish Singh <[email protected]> Signed-off-by: Ashish Singh <[email protected]> Signed-off-by: Ashish Singh <[email protected]> Co-authored-by: Ashish <[email protected]>
…5731) * Remove PRRL creation/deletion in peer recovery of remote store enabled replica (#4954) * Add RecoverySourceHandlerFactory for extensibility Signed-off-by: Ashish Singh <[email protected]> * recoverToTarget made extensible to allow multiple implementations Signed-off-by: Ashish Singh <[email protected]> * Remove PRRL after SendFileStep in Peer Recovery Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Empty-Commit Signed-off-by: Ashish Singh <[email protected]> * Remove CHANGELOG entry as this is incremental PR Signed-off-by: Ashish Singh <[email protected]> * Incorporate PR review feedback Signed-off-by: Ashish Singh <[email protected]> Signed-off-by: Ashish Singh <[email protected]> * Enhance CheckpointState to support no-op replication (#5282) * CheckpointState enhanced to support no-op replication Signed-off-by: Ashish Singh <[email protected]> Co-authored-by: Bukhtawar Khan<[email protected]> Signed-off-by: Ashish Singh <[email protected]> * Add transport action for primary term validation for remote-backed indices (#5616) Add transport action for primary term validation for remote-backed indices Signed-off-by: Ashish Singh <[email protected]> Signed-off-by: Ashish Singh <[email protected]>
Description
PRRL is used to keep lucene operation history and is also used in recovery for replaying these operations once the segment files are sent to the target. During replica recovery, PRRL is acquired for sending these operations to the replica after the segment files (from most recent safe commit) are copied over to replicas. This is not required when remote translog is enabled. This PR aims to remove the unnecessary code around PRRL in case of recovery of remote store enabled replicas.
Issues Resolved
#4502
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.