Refactoring checkpoint publishing to no longer be a Broadcast Action #2591
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
|
❌ Gradle Check failure 053ea1498474eec1a475e0e897815cb3b06cd078 |
mch2
approved these changes
Mar 24, 2022
| @@ -38,8 +38,8 @@ public void beforeRefresh() throws IOException { | |||
|
|
|||
| @Override | |||
| public void afterRefresh(boolean didRefresh) throws IOException { | |||
| if (shard.routingEntry().primary()) { | |||
| publisher.publish(shard.getLatestReplicationCheckpoint()); | |||
| if (shard.routingEntry().primary() && shard.indexSettings().isSegrepEnabled()) { | |||
There was a problem hiding this comment.
We conditionally wire up the refreshListener in IndexShard if segrep is enabled & the shard is a primary, I'm not sure we need these checks anymore?
There was a problem hiding this comment.
Sure, i can remove this. Follow up question - should we be checking didRefresh here before publishing the checkpoint ?
There was a problem hiding this comment.
good call, yes I think we should. Otherwise we could make multiple calls for the same ReplicationCheckpoint.
server/src/internalClusterTest/java/org/opensearch/index/shard/IndexShardIT.java
Show resolved
Hide resolved
This change adopts the same methodology as RetentionLeaseSyncer. A Transport client action is not required since we can skip the step of routing to the primary shard (the checkpoitn publisher is guaranteed to fire only from the primary). Now, checkpoint publishing directly invokes a TransportReplicationAction to perform the operation on the primary and replicas. PublishCheckpointAction has been reworked to be the TransportReplicationAction implementation rather than an ActionType. We leverage dependency injection to create the checkpoint publisher (and its internal action) at IndicesClusterStateService. This is plumed through to IndexShard which creates the refresh listener instance. All other transport layer classes tied to the original broadcast action are no longer required. Unrelated integration tests use a no-op/empty checkpoint publisher to satisfy their constructor/method argument. Signed-off-by: Kartik Ganesh <[email protected]>
kartg
force-pushed
the
feature/segment-replication
branch
from
053ea14 to
03bb12d
Compare
Poojita-Raj
approved these changes
Mar 25, 2022
|
|
||
| @Override | ||
| protected void doExecute(Task task, PublishCheckpointRequest request, ActionListener<ReplicationResponse> listener) { | ||
| assert false : "use PublishCheckpointAction#publish"; |
There was a problem hiding this comment.
What's the purpose of this function if it throws an assertion error everytime?
There was a problem hiding this comment.
Yeah, the implementation here isn't the clearest. What's going on is:
- To allow a primary shard to publish the checkpoint to its replicas, we're leveraging the ReplicationOperation functionality of executing on a primary followed by its replicas.
- In order to run a ReplicationOperation, we need to register an action name with the transport layer. TransportReplicationAction (the superclass here) does this in its constructor
- However, TransportReplicationAction also assumes that it will be called via execute/doExecute, which includes rerouting behavior. We don't need the reroute phase here since we are guaranteed to be on the primary shard
- Thus, the purpose of this implementation is to guard against accidental execution via the above code path. Instead, the refresh listener invokes the publish API directly
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
This change adopts the same methodology as
RetentionLeaseSyncer. A Transport client action is not required since we can skip the step of routing to the primary shard (the checkpoint publisher is guaranteed to fire only from the primary). Now, checkpoint publishing directly invokes aTransportReplicationActionto perform the operation on the primary and replicas.PublishCheckpointActionhas been reworked to be the TransportReplicationAction implementation rather than an ActionType.We leverage dependency injection to create the checkpoint publisher (and its internal action) at
IndicesClusterStateService. This is plumbed through toIndexShard-IndexShardcreates the refresh listener instance. All other transport layer classes tied to the original broadcast action are no longer required.Unrelated integration tests use a no-op/empty checkpoint publisher to satisfy their constructor/method argument.
Signed-off-by: Kartik Ganesh [email protected]
Issues Resolved
closes #2199
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.