Remove checking of sync commit ids (#114246)

arteam · web-flow · commit d93d333141fb · 2024-11-01T16:18:12.000+01:00
A Lucene commit doesn't contain sync ids `SegmentInfos` anymore, so we can't rely on them during recovery. The fields was marked as deprecated in #102343.
diff --git a/docs/reference/cat/shards.asciidoc b/docs/reference/cat/shards.asciidoc
@@ -9,13 +9,13 @@
 ====
 cat APIs are only intended for human consumption using the command line or {kib}
 console.
-They are _not_ intended for use by applications. For application 
+They are _not_ intended for use by applications. For application
 consumption, use the <<cluster-state,cluster state API>>.
 ====
 
 The `shards` command is the detailed view of all nodes' shard <<shard-allocation-relocation-recovery,allocation>>.
-It will tell you if the shard is a primary or replica, the number of docs, the 
-bytes it takes on disk, the node where it's located, and if the shard is 
+It will tell you if the shard is a primary or replica, the number of docs, the
+bytes it takes on disk, the node where it's located, and if the shard is
 currently <<shard-allocation-relocation-recovery,recovering>>.
 
 For <<data-streams,data streams>>, the API returns information about the stream's backing indices.
@@ -258,9 +258,6 @@ Time spent in suggest, such as `0`.
 `suggest.total`, `suto`, `suggestTotal`::
 Number of suggest operations, such as `0`.
 
-`sync_id`::
-Sync ID of the shard.
-
 `unassigned.at`, `ua`::
 Time at which the shard became unassigned in
 {wikipedia}/List_of_UTC_time_offsets[Coordinated Universal Time (UTC)].
diff --git a/rest-api-spec/build.gradle b/rest-api-spec/build.gradle
@@ -65,4 +65,5 @@ tasks.named("yamlRestCompatTestTransform").configure ({ task ->
   task.skipTest("indices.create/20_synthetic_source/nested object with unmapped fields", "temporary until backported")
   task.skipTest("indices.create/21_synthetic_source_stored/object param - nested object with stored array", "temporary until backported")
   task.skipTest("cat.aliases/10_basic/Deprecated local parameter", "CAT APIs not covered by compatibility policy")
+  task.skipTest("cat.shards/10_basic/Help", "sync_id is removed in 9.0")
 })
diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/cat.shards/10_basic.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/cat.shards/10_basic.yml
@@ -19,7 +19,6 @@
              ip                               .+   \n
              id                               .+   \n
              node                             .+   \n
-             sync_id                          .+   \n
              unassigned.reason                .+   \n
              unassigned.at                    .+   \n
              unassigned.for                   .+   \n
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/gateway/RecoveryFromGatewayIT.java b/server/src/internalClusterTest/java/org/elasticsearch/gateway/RecoveryFromGatewayIT.java
@@ -14,8 +14,6 @@
 import org.elasticsearch.action.admin.cluster.configuration.TransportAddVotingConfigExclusionsAction;
 import org.elasticsearch.action.admin.cluster.configuration.TransportClearVotingConfigExclusionsAction;
 import org.elasticsearch.action.admin.indices.recovery.RecoveryResponse;
-import org.elasticsearch.action.admin.indices.stats.IndexStats;
-import org.elasticsearch.action.admin.indices.stats.ShardStats;
 import org.elasticsearch.action.support.ActionTestUtils;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.coordination.ElectionSchedulerFactory;
@@ -30,7 +28,6 @@
 import org.elasticsearch.index.IndexService;
 import org.elasticsearch.index.IndexSettings;
 import org.elasticsearch.index.MergePolicyConfig;
-import org.elasticsearch.index.engine.Engine;
 import org.elasticsearch.index.query.QueryBuilders;
 import org.elasticsearch.index.shard.ShardId;
 import org.elasticsearch.index.shard.ShardPath;
@@ -577,13 +574,6 @@ public Settings onNodeStopped(String nodeName) throws Exception {
         }
     }
 
-    public void assertSyncIdsNotNull() {
-        IndexStats indexStats = indicesAdmin().prepareStats("test").get().getIndex("test");
-        for (ShardStats shardStats : indexStats.getShards()) {
-            assertNotNull(shardStats.getCommitStats().getUserData().get(Engine.SYNC_COMMIT_ID));
-        }
-    }
-
     public void testStartedShardFoundIfStateNotYetProcessed() throws Exception {
         // nodes may need to report the shards they processed the initial recovered cluster state from the master
         final String nodeName = internalCluster().startNode();
diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/NodeAllocationResult.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/NodeAllocationResult.java
@@ -210,19 +210,8 @@ public String getAllocationId() {
             return allocationId;
         }
 
-        /**
-         * Returns {@code true} if the shard copy has a matching sync id with the primary shard.
-         * Returns {@code false} if the shard copy does not have a matching sync id with the primary
-         * shard, or this explanation pertains to the allocation of a primary shard, in which case
-         * matching sync ids are irrelevant.
-         */
-        public boolean hasMatchingSyncId() {
-            return matchingBytes == Long.MAX_VALUE;
-        }
-
         /**
          * Gets the number of matching bytes the shard copy has with the primary shard.
-         * Returns {@code Long.MAX_VALUE} if {@link #hasMatchingSyncId()} returns {@code true}.
          * Returns -1 if not applicable (this value only applies to assigning replica shards).
          */
         public long getMatchingBytes() {
@@ -263,11 +252,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
                     builder.field("allocation_id", allocationId);
                 }
                 if (matchingBytes >= 0) {
-                    if (hasMatchingSyncId()) {
-                        builder.field("matching_sync_id", true);
-                    } else {
-                        builder.humanReadableField("matching_size_in_bytes", "matching_size", ByteSizeValue.ofBytes(matchingBytes));
-                    }
+                    builder.humanReadableField("matching_size_in_bytes", "matching_size", ByteSizeValue.ofBytes(matchingBytes));
                 }
                 if (storeException != null) {
                     builder.startObject("store_exception");
diff --git a/server/src/main/java/org/elasticsearch/gateway/ReplicaShardAllocator.java b/server/src/main/java/org/elasticsearch/gateway/ReplicaShardAllocator.java
@@ -439,14 +439,6 @@ private static long computeMatchingBytes(
         return sizeMatched;
     }
 
-    private static boolean hasMatchingSyncId(
-        TransportNodesListShardStoreMetadata.StoreFilesMetadata primaryStore,
-        TransportNodesListShardStoreMetadata.StoreFilesMetadata replicaStore
-    ) {
-        String primarySyncId = primaryStore.syncId();
-        return primarySyncId != null && primarySyncId.equals(replicaStore.syncId());
-    }
-
     private static MatchingNode computeMatchingNode(
         DiscoveryNode primaryNode,
         TransportNodesListShardStoreMetadata.StoreFilesMetadata primaryStore,
@@ -455,8 +447,7 @@ private static MatchingNode computeMatchingNode(
     ) {
         final long retainingSeqNoForPrimary = primaryStore.getPeerRecoveryRetentionLeaseRetainingSeqNo(primaryNode);
         final long retainingSeqNoForReplica = primaryStore.getPeerRecoveryRetentionLeaseRetainingSeqNo(replicaNode);
-        final boolean isNoopRecovery = (retainingSeqNoForReplica >= retainingSeqNoForPrimary && retainingSeqNoForPrimary >= 0)
-            || hasMatchingSyncId(primaryStore, replicaStore);
+        final boolean isNoopRecovery = (retainingSeqNoForReplica >= retainingSeqNoForPrimary && retainingSeqNoForPrimary >= 0);
         final long matchingBytes = computeMatchingBytes(primaryStore, replicaStore);
         return new MatchingNode(matchingBytes, retainingSeqNoForReplica, isNoopRecovery);
     }
@@ -470,9 +461,6 @@ private static boolean canPerformOperationBasedRecovery(
         if (targetNodeStore == null || targetNodeStore.storeFilesMetadata().isEmpty()) {
             return false;
         }
-        if (hasMatchingSyncId(primaryStore, targetNodeStore.storeFilesMetadata())) {
-            return true;
-        }
         return primaryStore.getPeerRecoveryRetentionLeaseRetainingSeqNo(targetNode) >= 0;
     }
 
diff --git a/server/src/main/java/org/elasticsearch/index/engine/Engine.java b/server/src/main/java/org/elasticsearch/index/engine/Engine.java
@@ -58,7 +58,6 @@
 import org.elasticsearch.core.Releasable;
 import org.elasticsearch.core.Releasables;
 import org.elasticsearch.core.TimeValue;
-import org.elasticsearch.core.UpdateForV9;
 import org.elasticsearch.index.IndexVersion;
 import org.elasticsearch.index.VersionType;
 import org.elasticsearch.index.mapper.DocumentParser;
@@ -117,8 +116,6 @@
 
 public abstract class Engine implements Closeable {
 
-    @UpdateForV9(owner = UpdateForV9.Owner.DISTRIBUTED_INDEXING) // TODO: Remove sync_id in 9.0
-    public static final String SYNC_COMMIT_ID = "sync_id";
     public static final String HISTORY_UUID_KEY = "history_uuid";
     public static final String FORCE_MERGE_UUID_KEY = "force_merge_uuid";
     public static final String MIN_RETAINED_SEQNO = "min_retained_seq_no";
diff --git a/server/src/main/java/org/elasticsearch/index/store/Store.java b/server/src/main/java/org/elasticsearch/index/store/Store.java
@@ -1150,14 +1150,6 @@ private int numSegmentFiles() { // only for asserts
             return count;
         }
 
-        /**
-         * Returns the sync id of the commit point that this MetadataSnapshot represents.
-         *
-         * @return sync id if exists, else null
-         */
-        public String getSyncId() {
-            return commitUserData.get(Engine.SYNC_COMMIT_ID);
-        }
     }
 
     /**
diff --git a/server/src/main/java/org/elasticsearch/indices/recovery/RecoverySourceHandler.java b/server/src/main/java/org/elasticsearch/indices/recovery/RecoverySourceHandler.java
@@ -534,56 +534,30 @@ void phase1(IndexCommit snapshot, long startingSeqNo, IntSupplier translogOps, A
                     );
                 }
             }
-            // When sync ids were used we could use them to check if two shard copies were equivalent,
-            // if that's the case we can skip sending files from the source shard to the target shard.
             // If the shard uses the current replication mechanism, we have to compute the recovery plan,
             // and it is still possible to skip the sending files from the source shard to the target shard
             // using a different mechanism to determine it.
-            // TODO: is this still relevant today?
-            if (hasSameLegacySyncId(recoverySourceMetadata, request.metadataSnapshot()) == false) {
-                cancellableThreads.checkForCancel();
-                SubscribableListener
-                    // compute the plan
-                    .<ShardRecoveryPlan>newForked(
-                        l -> recoveryPlannerService.computeRecoveryPlan(
-                            shard.shardId(),
-                            shardStateIdentifier,
-                            recoverySourceMetadata,
-                            request.metadataSnapshot(),
-                            startingSeqNo,
-                            translogOps.getAsInt(),
-                            getRequest().targetNode().getMaxIndexVersion(),
-                            canUseSnapshots(),
-                            request.isPrimaryRelocation(),
-                            l
-                        )
+            cancellableThreads.checkForCancel();
+            SubscribableListener
+                // compute the plan
+                .<ShardRecoveryPlan>newForked(
+                    l -> recoveryPlannerService.computeRecoveryPlan(
+                        shard.shardId(),
+                        shardStateIdentifier,
+                        recoverySourceMetadata,
+                        request.metadataSnapshot(),
+                        startingSeqNo,
+                        translogOps.getAsInt(),
+                        getRequest().targetNode().getMaxIndexVersion(),
+                        canUseSnapshots(),
+                        request.isPrimaryRelocation(),
+                        l
                     )
-                    // perform the file recovery
-                    .<SendFileResult>andThen((l, plan) -> recoverFilesFromSourceAndSnapshot(plan, store, stopWatch, l))
-                    // and respond
-                    .addListener(listener);
-            } else {
-                logger.trace("skipping [phase1] since source and target have identical sync id [{}]", recoverySourceMetadata.getSyncId());
-                SubscribableListener
-                    // but we must still create a retention lease
-                    .<RetentionLease>newForked(leaseListener -> createRetentionLease(startingSeqNo, leaseListener))
-                    // and then compute the result of sending no files
-                    .andThenApply(ignored -> {
-                        final TimeValue took = stopWatch.totalTime();
-                        logger.trace("recovery [phase1]: took [{}]", took);
-                        return new SendFileResult(
-                            Collections.emptyList(),
-                            Collections.emptyList(),
-                            0L,
-                            Collections.emptyList(),
-                            Collections.emptyList(),
-                            0L,
-                            took
-                        );
-                    })
-                    // and finally respond
-                    .addListener(listener);
-            }
+                )
+                // perform the file recovery
+                .<SendFileResult>andThen((l, plan) -> recoverFilesFromSourceAndSnapshot(plan, store, stopWatch, l))
+                // and respond
+                .addListener(listener);
         } catch (Exception e) {
             throw new RecoverFilesRecoveryException(request.shardId(), 0, ByteSizeValue.ZERO, e);
         }
@@ -1030,43 +1004,6 @@ private ActionListener<ReplicationResponse> wrapLeaseSyncListener(ActionListener
         return new ThreadedActionListener<>(shard.getThreadPool().generic(), listener).map(ignored -> null);
     }
 
-    boolean hasSameLegacySyncId(Store.MetadataSnapshot source, Store.MetadataSnapshot target) {
-        if (source.getSyncId() == null || source.getSyncId().equals(target.getSyncId()) == false) {
-            return false;
-        }
-        if (source.numDocs() != target.numDocs()) {
-            throw new IllegalStateException(
-                "try to recover "
-                    + request.shardId()
-                    + " from primary shard with sync id but number "
-                    + "of docs differ: "
-                    + source.numDocs()
-                    + " ("
-                    + request.sourceNode().getName()
-                    + ", primary) vs "
-                    + target.numDocs()
-                    + "("
-                    + request.targetNode().getName()
-                    + ")"
-            );
-        }
-        SequenceNumbers.CommitInfo sourceSeqNos = SequenceNumbers.loadSeqNoInfoFromLuceneCommit(source.commitUserData().entrySet());
-        SequenceNumbers.CommitInfo targetSeqNos = SequenceNumbers.loadSeqNoInfoFromLuceneCommit(target.commitUserData().entrySet());
-        if (sourceSeqNos.localCheckpoint() != targetSeqNos.localCheckpoint() || targetSeqNos.maxSeqNo() != sourceSeqNos.maxSeqNo()) {
-            final String message = "try to recover "
-                + request.shardId()
-                + " with sync id but "
-                + "seq_no stats are mismatched: ["
-                + source.commitUserData()
-                + "] vs ["
-                + target.commitUserData()
-                + "]";
-            assert false : message;
-            throw new IllegalStateException(message);
-        }
-        return true;
-    }
-
     void prepareTargetForTranslog(int totalTranslogOps, ActionListener<TimeValue> listener) {
         StopWatch stopWatch = new StopWatch().start();
         final ActionListener<Void> wrappedListener = ActionListener.wrap(nullVal -> {
diff --git a/server/src/main/java/org/elasticsearch/indices/store/TransportNodesListShardStoreMetadata.java b/server/src/main/java/org/elasticsearch/indices/store/TransportNodesListShardStoreMetadata.java
@@ -259,22 +259,9 @@ public long getPeerRecoveryRetentionLeaseRetainingSeqNo(DiscoveryNode node) {
                 .orElse(-1L);
         }
 
-        /**
-         * @return commit sync id if exists, else null
-         */
-        public String syncId() {
-            return metadataSnapshot.getSyncId();
-        }
-
         @Override
         public String toString() {
-            return "StoreFilesMetadata{"
-                + ", metadataSnapshot{size="
-                + metadataSnapshot.size()
-                + ", syncId="
-                + metadataSnapshot.getSyncId()
-                + "}"
-                + '}';
+            return "StoreFilesMetadata{" + ", metadataSnapshot{size=" + metadataSnapshot.size() + "}" + '}';
         }
     }
 
diff --git a/server/src/main/java/org/elasticsearch/rest/action/cat/RestShardsAction.java b/server/src/main/java/org/elasticsearch/rest/action/cat/RestShardsAction.java
@@ -23,11 +23,11 @@
 import org.elasticsearch.common.Table;
 import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.common.util.concurrent.ListenableFuture;
+import org.elasticsearch.core.RestApiVersion;
 import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.index.bulk.stats.BulkStats;
 import org.elasticsearch.index.cache.query.QueryCacheStats;
 import org.elasticsearch.index.engine.CommitStats;
-import org.elasticsearch.index.engine.Engine;
 import org.elasticsearch.index.engine.SegmentsStats;
 import org.elasticsearch.index.fielddata.FieldDataStats;
 import org.elasticsearch.index.flush.FlushStats;
@@ -123,7 +123,9 @@ protected Table getTableWithHeader(final RestRequest request) {
             .addCell("id", "default:false;desc:unique id of node where it lives")
             .addCell("node", "default:true;alias:n;desc:name of node where it lives");
 
-        table.addCell("sync_id", "alias:sync_id;default:false;desc:sync id");
+        if (request.getRestApiVersion() == RestApiVersion.V_8) {
+            table.addCell("sync_id", "alias:sync_id;default:false;desc:sync id");
+        }
 
         table.addCell("unassigned.reason", "alias:ur;default:false;desc:reason shard became unassigned");
         table.addCell("unassigned.at", "alias:ua;default:false;desc:time shard became unassigned (UTC)");
@@ -320,7 +322,9 @@ Table buildTable(RestRequest request, ClusterStateResponse state, IndicesStatsRe
                 table.addCell(null);
             }
 
-            table.addCell(commitStats == null ? null : commitStats.getUserData().get(Engine.SYNC_COMMIT_ID));
+            if (request.getRestApiVersion() == RestApiVersion.V_8) {
+                table.addCell(null);
+            }
 
             if (shard.unassignedInfo() != null) {
                 table.addCell(shard.unassignedInfo().reason());
diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/NodeAllocationResultTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/NodeAllocationResultTests.java
@@ -48,7 +48,6 @@ public void testShardStore() throws IOException {
         assertEquals(matchingBytes, explanation.getShardStoreInfo().getMatchingBytes());
         assertNull(explanation.getShardStoreInfo().getAllocationId());
         assertFalse(explanation.getShardStoreInfo().isInSync());
-        assertFalse(explanation.getShardStoreInfo().hasMatchingSyncId());
 
         String allocId = randomAlphaOfLength(5);
         boolean inSync = randomBoolean();
@@ -60,7 +59,6 @@ public void testShardStore() throws IOException {
         assertNodeExplanationEquals(explanation, readExplanation);
         assertEquals(inSync, explanation.getShardStoreInfo().isInSync());
         assertEquals(-1, explanation.getShardStoreInfo().getMatchingBytes());
-        assertFalse(explanation.getShardStoreInfo().hasMatchingSyncId());
         assertEquals(allocId, explanation.getShardStoreInfo().getAllocationId());
     }
 
@@ -72,7 +70,6 @@ private void assertNodeExplanationEquals(NodeAllocationResult expl1, NodeAllocat
             assertEquals(expl1.getShardStoreInfo().isInSync(), expl2.getShardStoreInfo().isInSync());
             assertEquals(expl1.getShardStoreInfo().getAllocationId(), expl2.getShardStoreInfo().getAllocationId());
             assertEquals(expl1.getShardStoreInfo().getMatchingBytes(), expl2.getShardStoreInfo().getMatchingBytes());
-            assertEquals(expl1.getShardStoreInfo().hasMatchingSyncId(), expl2.getShardStoreInfo().hasMatchingSyncId());
         } else {
             assertNull(expl2.getShardStoreInfo());
         }
diff --git a/server/src/test/java/org/elasticsearch/gateway/ReplicaShardAllocatorTests.java b/server/src/test/java/org/elasticsearch/gateway/ReplicaShardAllocatorTests.java
diff --git a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java b/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java
diff --git a/server/src/test/java/org/elasticsearch/index/store/StoreTests.java b/server/src/test/java/org/elasticsearch/index/store/StoreTests.java
diff --git a/server/src/test/java/org/elasticsearch/indices/recovery/RecoverySourceHandlerTests.java b/server/src/test/java/org/elasticsearch/indices/recovery/RecoverySourceHandlerTests.java
diff --git a/server/src/test/java/org/elasticsearch/rest/action/cat/RestShardsActionTests.java b/server/src/test/java/org/elasticsearch/rest/action/cat/RestShardsActionTests.java
diff --git a/x-pack/plugin/stack/qa/rest/build.gradle b/x-pack/plugin/stack/qa/rest/build.gradle

Original file line number	Diff line number	Diff line change
`@@ -1150,14 +1150,6 @@ private int numSegmentFiles() { // only for asserts`
`1150`	`1150`	`return count;`
`1151`	`1151`	`}`
`1152`	`1152`
`1153`		`- /**`
`1154`		`- * Returns the sync id of the commit point that this MetadataSnapshot represents.`
`1155`		`- *`
`1156`		`- * @return sync id if exists, else null`
`1157`		`- */`
`1158`		`- public String getSyncId() {`
`1159`		`- return commitUserData.get(Engine.SYNC_COMMIT_ID);`
`1160`		`- }`
`1161`	`1153`	`}`
`1162`	`1154`
`1163`	`1155`	`/**`