Refactoring of Segment Replication classes #2468
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since this class wraps two subclasses of RefCounted, and has the same shutdown hook for both (decRef), I've made the functionality of the class less generic to increase code convergence. Signed-off-by: Kartik Ganesh <[email protected]>
Parent class names are currently temporary placeholders. They will renamed in a separate commit to avoid a convoluted commit diff. A large part of duplicate functionality in the two *Target classes has been moved into the parent class. The two *Listeners are also very similar and now implement a common interface. The usec-ase specific methods in the listeners will eventually be replaced by their generic equivalents. Finally, the State parent class is currently a placeholder and will soon hold more logic. Some other parts of the code were also changed to make call patterns more streamlined. Signed-off-by: Kartik Ganesh <[email protected]>
Signed-off-by: Kartik Ganesh <[email protected]>
The existing ReplicationTarget has been renamed to SegmentReplicationTarget. Alongside this change, unnecessary wrapper methods have been removed and replaced with more direct invocations. Signed-off-by: Kartik Ganesh <[email protected]>
The ReplicationListener class has been renamed to SegmentReplicationListener, and the parent class now takes the more-generic ReplicationListener name. The inner classes in IndicesClusterStateService have also been renamed to ShardRouting* to reflect their purpose. Signed-off-by: Kartik Ganesh <[email protected]>
This class is currently across both recovery and segment replication. It has two dependent inner classes - RecoveryFilesDetails and FileDetail - which have been moved from RecoveryState to be inner classes in RecoveryIndex Signed-off-by: Kartik Ganesh <[email protected]>
A small amount of common members and logic from its subclasses has been moved into the parent class. The original ReplicationState child class has been renamed SegmentReplicationState. Signed-off-by: Kartik Ganesh <[email protected]>
|
Can one of the admins verify this patch? |
mch2
reviewed
Mar 15, 2022
| @@ -748,7 +748,7 @@ private static DiscoveryNode findSourceNodeForPeerRecovery( | |||
| return sourceNode; | |||
| } | |||
|
|
|||
| private class ReplicationListener implements SegmentReplicationReplicaService.ReplicationListener { | |||
| private class ShardRoutingReplicationListener implements SegmentReplicationReplicaService.SegmentReplicationListener { | |||
There was a problem hiding this comment.
This was me being quick and dirty with the POC - I don't think we need a separate listener type for segrep. I think we could extract PeerRecoveryTargetService.RecoveryListener and use it for segrep with a common exception type. We will still need to perform the same actions that RecoveryListener now performs on success (publishing shardStarted) and failure (failing and removing the shard).
There was a problem hiding this comment.
Understood. I'll punt this to a future refactor
| @@ -35,7 +35,7 @@ | |||
| import org.apache.logging.log4j.Logger; | |||
There was a problem hiding this comment.
Should we also rename this class to SegmentReplicationCollection?
There was a problem hiding this comment.
I haven't yet touched the Collection classes so i'd like to leave this as-is for now. In a follow up refactor, we can rename this appropriately after synergizing functionality with the Recovery equivalent
server/src/main/java/org/opensearch/common/concurrent/AutoCloseableRefCounted.java
Outdated
Show resolved
Hide resolved
This is in response to PR feedback, and helps avoid a verbose get() method by introducing the generic at the class level. Signed-off-by: Kartik Ganesh <[email protected]>
31 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Incremental refactoring of Segment Replication classes to centralize common functionality between seg-rep and recovery code. These are mostly modeled as
Replication*parent classes withRecovery*andSegmentReplication*child classes.There is much more refactoring that can be done above this, but the work in these commits is a time-boxed effort.
Issues Resolved
[List any issues this PR will resolve]
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.