Dual Replication - Primary changes #152

shourya035 · 2024-03-08T09:01:02Z

Description

[Describe what this change achieves]

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)
Public documentation issue/PR created

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.

gbbafna · 2024-03-11T10:37:37Z

server/src/main/java/org/opensearch/index/shard/IndexShard.java

@@ -5199,6 +5209,10 @@ public AsyncIOProcessor<Translog.Location> getTranslogSyncProcessor() {
        return translogSyncProcessor;
    }

+    public boolean ongoingEngineMigration() {
+        return RemoteStoreUtils.isMigrationDirectionSet(clusterService) == true;


This will return true for all the shards during migration . Will that have any side effects ?

Removed this altogether from IndexShard

gbbafna · 2024-03-11T10:40:50Z

server/src/main/java/org/opensearch/index/shard/CheckpointRefreshListener.java

+            && !shard.indexSettings.isSegRepWithRemoteEnabled()
+            /*
+             During remote store migration, the isSegRepWithRemoteEnabled criteria would return false
+             since we do not alter the remote store based index settings at that stage. Explicitly
+             blocking checkpoint publication from this refresh listener since it ends up interfering
+             with the RemoteStoreRefreshListener invocation
+             */
+            && !shard.ongoingEngineMigration()) {


can we have isSegRepLocalEnabled instead of negative checks ?

Addressed this. Changed the isSegRepLocalEnabled check to ensure that the Checkpoint listeners won't be registered if the node is non-remote

gbbafna · 2024-03-11T10:52:53Z

server/src/main/java/org/opensearch/index/seqno/ReplicationTracker.java

@@ -1248,7 +1254,8 @@ private void createReplicationLagTimers() {
                if (cps.inSync
                    && replicationGroup.getUnavailableInSyncShards().contains(allocationId) == false
                    && isPrimaryRelocation(allocationId) == false
-                    && latestReplicationCheckpoint.isAheadOf(cps.visibleReplicationCheckpoint)) {
+                    && latestReplicationCheckpoint.isAheadOf(cps.visibleReplicationCheckpoint)
+                    && (ongoingEngineMigration && routingTable.getByAllocationId(allocationId).isAssignedToRemoteStoreNode() == true)) {


why do we need ongoingEngineMigration here ?

Removed this. Relying on the shardRouting and isSegRepLocalEnabled check to ensure that the timers are only created if:

The shard copy is hosted on a remote enabled node

The index setting has segrep enabled but remote disabled (vanilla segrep)

gbbafna · 2024-03-11T13:05:36Z

server/src/main/java/org/opensearch/indices/cluster/IndicesClusterStateService.java

@@ -1096,4 +1098,14 @@ enum IndexRemovalReason {
            REOPENED,
        }
    }
+
+    private void updateShardRoutingWithNode(IndexShardRoutingTable indexShardRoutingTable, DiscoveryNodes nodes) {


what about createShard flow ?

Addressing that

gbbafna · 2024-03-11T13:07:15Z

server/src/main/java/org/opensearch/cluster/routing/ShardRouting.java

@@ -75,6 +75,7 @@ public class ShardRouting implements Writeable, ToXContentObject {
    private final long expectedShardSize;
    @Nullable
    private final ShardRouting targetRelocatingShard;
+    private boolean assignedToRemoteStoreNode;


this is not populated by master and only data node . Is it intentional ? If yes, should we make it Boolean instead ? Also it is final .

Ideally we don't want to serialize the value for this field since this won't be used anywhere else. I have changed this to Boolean instead of the primitive type, but I have assigned a default value to prevent any possible NPEs when the shardRouting entry sent from the cluster manager nodes are serialized on the data node.

gbbafna · 2024-03-11T13:15:44Z

server/src/main/java/org/opensearch/action/support/replication/ReplicationModeAwareProxy.java

        // If the current routing is the primary, then it does not need to be replicated
        if (shardRouting.isSameAllocation(primaryRouting)) {
            return ReplicationMode.NO_REPLICATION;
        }
-
+        // Perform full replication during primary failover


primary relocation.

ashking94 · 2024-03-18T04:37:47Z

server/src/main/java/org/opensearch/action/support/replication/TransportReplicationAction.java

@@ -642,7 +642,7 @@ public void handleException(TransportException exp) {
                        primaryRequest.getPrimaryTerm(),
                        initialRetryBackoffBound,
                        retryTimeout,
-                        indexShard.isRemoteTranslogEnabled()
+                        indexShard.isRemoteTranslogEnabled() || indexShard.indexSettings().isRemoteNode()


Lets subsume the check of indexShard.indexSettings().isRemoteNode() inside the indexShard.isRemoteTranslogEnabled() check itself?

ashking94 · 2024-03-18T04:44:20Z

server/src/main/java/org/opensearch/index/seqno/ReplicationTracker.java

@@ -1089,10 +1089,6 @@ private ReplicationGroup calculateReplicationGroup() {
            newVersion = replicationGroup.getVersion() + 1;
        }

-        assert indexSettings().isRemoteTranslogStoreEnabled()


Lets not remove this assertion. Removing this can pose risks to bugs.

We can check first if index is remote enabled, or if there is an inflight migration, or all tracked shards are replicated.

ashking94 · 2024-03-18T04:51:26Z

server/src/main/java/org/opensearch/index/seqno/ReplicationTracker.java

+                    && latestReplicationCheckpoint.isAheadOf(cps.visibleReplicationCheckpoint)
+                    /*
+                     Handle remote store migration cases. Replication Lag timers would be created if the node on which primary is hosted is either:
+                     - Segrep enabled without remote store
+                     - Destination replica shard is hosted on a remote store enabled node (Remote store enabled nodes have segrep enabled implicitly)
+                    */
+                    && (indexSettings.isSegRepLocalEnabled() == true
+                        || routingTable.getByAllocationId(allocationId).isAssignedToRemoteStoreNode() == true)) {


Not really clear on why is this being added?

ashking94 · 2024-03-18T04:56:12Z

server/src/main/java/org/opensearch/index/seqno/ReplicationTracker.java

+         - If remote translog is enabled, then returns true if given allocation id matches the primary or it's relocation target allocation primary and primary target allocation id.
+         - During an ongoing remote migration, the above-mentioned checks are considered when the shard is assigned to a remote store backed node
+         */
+        if (indexSettings().isRemoteTranslogStoreEnabled() || assignedToRemoteStoreNode == true) {
            return (allocationId.equals(primaryAllocationId) || allocationId.equals(primaryTargetAllocationId));


can we return false if the if condition holds true since the remote translogs would anyways be downloaded on the relocating primary?

ashking94 · 2024-03-18T04:57:14Z

server/src/main/java/org/opensearch/index/seqno/RetentionLeaseBackgroundSyncAction.java

+        if (RemoteStoreUtils.isMigrationDirectionSet(clusterService)) {
+            return ReplicationMode.FULL_REPLICATION;
+        }


Why do we need it on shards that are on remote store already?

Same question applicable for other actions as well.

ashking94 · 2024-03-18T04:57:51Z

server/src/main/java/org/opensearch/index/shard/IndexShard.java

@@ -3328,7 +3333,7 @@ public void syncRetentionLeases() {
                )
            );
        } else {
-            logger.trace("background syncing retention leases [{}] after expiration check", retentionLeases.v2());
+            logger.info("background syncing retention leases [{}] after expiration check", retentionLeases.v2());


do we need this with info level?

ashking94 · 2024-03-18T05:00:16Z

server/src/main/java/org/opensearch/index/shard/IndexShard.java

+        // Explicitly disable search idle during remote store migration
+        if (RemoteStoreUtils.isMigrationDirectionSet(clusterService)) {
+            return false;
+        }


Curious on why is this required?

ashking94 · 2024-03-18T05:00:38Z

server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java

@@ -340,7 +340,7 @@ private void onSuccessfulSegmentsSync(
        resetBackOffDelayIterator();
        // Set the minimum sequence number for keeping translog
        indexShard.getEngine().translogManager().setMinSeqNoToKeep(lastRefreshedCheckpoint + 1);
-        // Publishing the new checkpoint which is used for remote store + segrep indexes
+        // Publishing the new checkpoint which is used for remote store + segrep indexess


lets remove this unintended change?

ashking94 · 2024-03-18T05:01:29Z

server/src/main/java/org/opensearch/indices/cluster/IndicesClusterStateService.java

+            DiscoveryNode targetNode = nodes.getLocalNode();
+            // Set remote attributes on Shard routing if the target node for the shards has remote attributes
+            if (targetNode.isRemoteStoreNode()) {
+                shardRouting.setAssignedToRemoteStoreNode(true);


appears to be error prone, is there a way to have this as final and set inside ctor?

ashking94 · 2024-03-18T05:02:33Z

server/src/main/java/org/opensearch/indices/cluster/IndicesClusterStateService.java

@@ -1096,4 +1102,14 @@ enum IndexRemovalReason {
            REOPENED,
        }
    }
+
+    private void updateShardRoutingWithNode(IndexShardRoutingTable indexShardRoutingTable, DiscoveryNodes nodes) {


is there a way this can be done within ctor only?

ashking94

Left some comments, please take a look at them.

Signed-off-by: Shourya Dutta Biswas <[email protected]>

gbbafna reviewed Mar 11, 2024

View reviewed changes

shourya035 force-pushed the dual-rep-pri branch from 5dbae4f to 7d9f9cd Compare March 13, 2024 12:33

gbbafna force-pushed the mixed-relocation branch from 463eb84 to 77c9b3a Compare March 15, 2024 05:18

shourya035 force-pushed the dual-rep-pri branch from 7d9f9cd to 791e2da Compare March 15, 2024 08:42

ashking94 reviewed Mar 18, 2024

View reviewed changes

ashking94 requested changes Mar 18, 2024

View reviewed changes

gbbafna force-pushed the mixed-relocation branch from 68e9d38 to 2a828e5 Compare March 18, 2024 10:50

shourya035 force-pushed the dual-rep-pri branch from f32ee23 to 561375c Compare March 19, 2024 13:20

gbbafna force-pushed the mixed-relocation branch from cf5dc23 to 5601758 Compare March 21, 2024 04:03

shourya035 added 3 commits March 21, 2024 10:43

Primary changes to support dual mode replication

5e66c19

Signed-off-by: Shourya Dutta Biswas <[email protected]>

Addressing comments

24e3975

Signed-off-by: Shourya Dutta Biswas <[email protected]>

Running Spotless checks

d3ada3a

Signed-off-by: Shourya Dutta Biswas <[email protected]>

shourya035 force-pushed the dual-rep-pri branch from 561375c to d3ada3a Compare March 21, 2024 05:17

shourya035 closed this Mar 21, 2024

shourya035 deleted the dual-rep-pri branch March 21, 2024 05:26

shourya035 mentioned this pull request Mar 22, 2024

[Remote Store] Primary/Replica side changes to support Dual Replication opensearch-project/OpenSearch#12821

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dual Replication - Primary changes #152

Dual Replication - Primary changes #152

shourya035 commented Mar 8, 2024 •

edited

Loading

gbbafna Mar 11, 2024

shourya035 Mar 13, 2024

gbbafna Mar 11, 2024

shourya035 Mar 13, 2024

gbbafna Mar 11, 2024

shourya035 Mar 13, 2024

gbbafna Mar 11, 2024

shourya035 Mar 13, 2024

gbbafna Mar 11, 2024

shourya035 Mar 13, 2024

gbbafna Mar 11, 2024

ashking94 Mar 18, 2024

ashking94 Mar 18, 2024

ashking94 Mar 18, 2024

ashking94 Mar 18, 2024

ashking94 Mar 18, 2024

ashking94 Mar 18, 2024

ashking94 Mar 18, 2024

ashking94 Mar 18, 2024

ashking94 Mar 18, 2024

ashking94 Mar 18, 2024

ashking94 Mar 18, 2024

ashking94 left a comment

Dual Replication - Primary changes #152

Dual Replication - Primary changes #152

Conversation

shourya035 commented Mar 8, 2024 • edited Loading

Description

Related Issues

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashking94 left a comment

Choose a reason for hiding this comment

shourya035 commented Mar 8, 2024 •

edited

Loading