Avoid changing tsid creation strategy for an index (#135514)

This avoids that the tsid and routing is calculated differently on the same index.
Doing so would have the following consequences:
* De-duplication would not work, for example, when a client re-transmits a batch as the id (which is based on the tsid) will be different.
* When replaying a translog operation, the re-computed tsid and id will differ  from the id stored in the translog. This would lead to a failure of the index operation.

Author: felixbarny

modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBIndexingIT.java

@@ -8,6 +8,7 @@
  */
 package org.elasticsearch.datastreams;
 
+import org.elasticsearch.action.ActionFuture;
 import org.elasticsearch.action.DocWriteRequest;
 import org.elasticsearch.action.admin.indices.diskusage.AnalyzeIndexDiskUsageRequest;
 import org.elasticsearch.action.admin.indices.diskusage.TransportAnalyzeIndexDiskUsageAction;
@@ -31,6 +32,7 @@
 import org.elasticsearch.action.index.IndexRequest;
 import org.elasticsearch.action.search.SearchRequest;
 import org.elasticsearch.action.support.WriteRequest;
+import org.elasticsearch.action.support.master.AcknowledgedResponse;
 import org.elasticsearch.cluster.metadata.ComponentTemplate;
 import org.elasticsearch.cluster.metadata.ComposableIndexTemplate;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
@@ -709,17 +711,20 @@ public void testAddDimensionToMapping() throws Exception {
               ]
             }
             """, XContentType.JSON);
-        assertAcked(client().execute(TransportPutMappingAction.TYPE, putMappingRequest).actionGet());
+        ActionFuture<AcknowledgedResponse> putMappingFuture = client().execute(TransportPutMappingAction.TYPE, putMappingRequest);
         if (INDEX_DIMENSIONS_TSID_OPTIMIZATION_FEATURE_FLAG) {
+            IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, putMappingFuture::actionGet);
             assertThat(
-                getSetting(dataStreamName, IndexMetadata.INDEX_ROUTING_PATH),
-                containsInAnyOrder("metricset", "labels.*", "k8s.pod.name")
+                exception.getMessage(),
+                containsString("Cannot add dynamic templates that define dimension fields on an existing index with index.dimensions")
             );
+            assertThat(getSetting(dataStreamName, IndexMetadata.INDEX_DIMENSIONS), containsInAnyOrder("metricset", "k8s.pod.name"));
+            assertThat(getSetting(dataStreamName, IndexMetadata.INDEX_ROUTING_PATH), empty());
         } else {
+            assertAcked(putMappingFuture);
             assertThat(getSetting(dataStreamName, IndexMetadata.INDEX_ROUTING_PATH), containsInAnyOrder("metricset"));
+            assertThat(getSetting(dataStreamName, IndexMetadata.INDEX_DIMENSIONS), empty());
         }
-        assertThat(getSetting(dataStreamName, IndexMetadata.INDEX_DIMENSIONS), empty());
-
         indexWithPodNames(dataStreamName, Instant.now(), Map.of(), "dog", "cat");
     }
 

modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java

@@ -154,11 +154,15 @@ public void provideAdditionalSettings(
     }
 
     /**
-     * This is called when mappings are updated, so that the {@link IndexMetadata#getTimeSeriesDimensions()}
-     * and {@link IndexMetadata#INDEX_ROUTING_PATH} settings are updated to match the new mappings.
-     * Updates {@link IndexMetadata#getTimeSeriesDimensions} if a new dimension field is added to the mappings,
-     * or sets {@link IndexMetadata#INDEX_ROUTING_PATH} if a new dimension field is added that doesn't allow for matching all
-     * dimension fields via a wildcard pattern.
+     * This is called when mappings are updated, so that the {@link IndexMetadata#INDEX_DIMENSIONS}
+     * setting is updated if a new dimension field is added to the mappings.
+     *
+     * @throws IllegalArgumentException If a dynamic template that defines dimension fields is added to an existing index with
+     *                                  {@link IndexMetadata#getTimeSeriesDimensions()}.
+     *                                  Changing fom {@link IndexMetadata#INDEX_DIMENSIONS} to {@link IndexMetadata#INDEX_ROUTING_PATH}
+     *                                  is not allowed because it would violate the invariant that the same input document always results
+     *                                  in the same _id and _tsid.
+     *                                  Otherwise, data duplication or translog replay issues could occur.
      */
     @Override
     public void onUpdateMappings(IndexMetadata indexMetadata, DocumentMapper documentMapper, Settings.Builder additionalSettings) {
@@ -172,11 +176,13 @@ public void onUpdateMappings(IndexMetadata indexMetadata, DocumentMapper documen
         boolean hasChanges = indexDimensions.size() != newIndexDimensions.size()
             && new HashSet<>(indexDimensions).equals(new HashSet<>(newIndexDimensions)) == false;
         if (matchesAllDimensions == false) {
-            // If the new dimensions don't match all potential dimension fields, we need to unset index.dimensions
-            // and set index.routing_path instead.
-            // This can happen if a new dynamic template with time_series_dimension: true is added to an existing index.
-            additionalSettings.putList(INDEX_DIMENSIONS.getKey(), List.of());
-            additionalSettings.putList(INDEX_ROUTING_PATH.getKey(), newIndexDimensions);
+            throw new IllegalArgumentException(
+                "Cannot add dynamic templates that define dimension fields on an existing index with "
+                    + INDEX_DIMENSIONS.getKey()
+                    + ". "
+                    + "Please change the index template and roll over the data stream "
+                    + "instead of modifying the mappings of the backing indices."
+            );
         } else if (hasChanges) {
             additionalSettings.putList(INDEX_DIMENSIONS.getKey(), newIndexDimensions);
         }

modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProviderTests.java

@@ -956,10 +956,18 @@ public void testAddDynamicTemplate() throws Exception {
             }
             """;
         // we don't support index.dimensions with dynamic templates so we'll unset index.dimensions
-        Settings result = onUpdateMappings(null, "labels.*", mapping);
-        assertThat(result.size(), equalTo(2));
-        assertThat(IndexMetadata.INDEX_DIMENSIONS.get(result), empty());
-        assertThat(IndexMetadata.INDEX_ROUTING_PATH.get(result), containsInAnyOrder("labels.*"));
+        IllegalArgumentException exception = assertThrows(
+            IllegalArgumentException.class,
+            () -> onUpdateMappings(null, "labels.*", mapping)
+        );
+        assertThat(
+            exception.getMessage(),
+            equalTo(
+                "Cannot add dynamic templates that define dimension fields on an existing index with index.dimensions. "
+                    + "Please change the index template and roll over the data stream "
+                    + "instead of modifying the mappings of the backing indices."
+            )
+        );
     }
 
     private Settings generateTsdbSettings(String mapping, Instant now) throws IOException {