Make UpdateSettingsClusterStateUpdateRequest a record (#113450)

DaveCTurner · web-flow · commit ca4bf1a6b971 · 2024-09-25T02:06:34.000+10:00
No need to extend `IndicesClusterStateUpdateRequest`, this thing can be
completely immutable.
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsServiceIT.java b/server/src/internalClusterTest/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsServiceIT.java
@@ -28,6 +28,7 @@
 import java.util.List;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Function;
 
 import static org.hamcrest.Matchers.equalTo;
 
@@ -42,45 +43,58 @@ public void testThatNonDynamicSettingChangesTakeEffect() throws Exception {
         MetadataUpdateSettingsService metadataUpdateSettingsService = internalCluster().getCurrentMasterNodeInstance(
             MetadataUpdateSettingsService.class
         );
-        UpdateSettingsClusterStateUpdateRequest request = new UpdateSettingsClusterStateUpdateRequest().ackTimeout(TimeValue.ZERO);
-        List<Index> indices = new ArrayList<>();
+        List<Index> indicesList = new ArrayList<>();
         for (IndicesService indicesService : internalCluster().getInstances(IndicesService.class)) {
             for (IndexService indexService : indicesService) {
-                indices.add(indexService.index());
+                indicesList.add(indexService.index());
             }
         }
-        request.indices(indices.toArray(Index.EMPTY_ARRAY));
-        request.settings(Settings.builder().put("index.codec", "FastDecompressionCompressingStoredFieldsData").build());
+        final var indices = indicesList.toArray(Index.EMPTY_ARRAY);
+
+        final Function<UpdateSettingsClusterStateUpdateRequest.OnStaticSetting, UpdateSettingsClusterStateUpdateRequest> requestFactory =
+            onStaticSetting -> new UpdateSettingsClusterStateUpdateRequest(
+                TEST_REQUEST_TIMEOUT,
+                TimeValue.ZERO,
+                Settings.builder().put("index.codec", "FastDecompressionCompressingStoredFieldsData").build(),
+                UpdateSettingsClusterStateUpdateRequest.OnExisting.OVERWRITE,
+                onStaticSetting,
+                indices
+            );
 
         // First make sure it fails if reopenShards is not set on the request:
         AtomicBoolean expectedFailureOccurred = new AtomicBoolean(false);
-        metadataUpdateSettingsService.updateSettings(request, new ActionListener<>() {
-            @Override
-            public void onResponse(AcknowledgedResponse acknowledgedResponse) {
-                fail("Should have failed updating a non-dynamic setting without reopenShards set to true");
-            }
+        metadataUpdateSettingsService.updateSettings(
+            requestFactory.apply(UpdateSettingsClusterStateUpdateRequest.OnStaticSetting.REJECT),
+            new ActionListener<>() {
+                @Override
+                public void onResponse(AcknowledgedResponse acknowledgedResponse) {
+                    fail("Should have failed updating a non-dynamic setting without reopenShards set to true");
+                }
 
-            @Override
-            public void onFailure(Exception e) {
-                expectedFailureOccurred.set(true);
+                @Override
+                public void onFailure(Exception e) {
+                    expectedFailureOccurred.set(true);
+                }
             }
-        });
+        );
         assertBusy(() -> assertThat(expectedFailureOccurred.get(), equalTo(true)));
 
         // Now we set reopenShards and expect it to work:
-        request.reopenShards(true);
         AtomicBoolean success = new AtomicBoolean(false);
-        metadataUpdateSettingsService.updateSettings(request, new ActionListener<>() {
-            @Override
-            public void onResponse(AcknowledgedResponse acknowledgedResponse) {
-                success.set(true);
-            }
+        metadataUpdateSettingsService.updateSettings(
+            requestFactory.apply(UpdateSettingsClusterStateUpdateRequest.OnStaticSetting.REOPEN_INDICES),
+            new ActionListener<>() {
+                @Override
+                public void onResponse(AcknowledgedResponse acknowledgedResponse) {
+                    success.set(true);
+                }
 
-            @Override
-            public void onFailure(Exception e) {
-                fail(e);
+                @Override
+                public void onFailure(Exception e) {
+                    fail(e);
+                }
             }
-        });
+        );
         assertBusy(() -> assertThat(success.get(), equalTo(true)));
 
         // Now we look into the IndexShard objects to make sure that the code was actually updated (vs just the setting):
@@ -110,16 +124,23 @@ public void testThatNonDynamicSettingChangesDoNotUnncessesarilyCauseReopens() th
         MetadataUpdateSettingsService metadataUpdateSettingsService = internalCluster().getCurrentMasterNodeInstance(
             MetadataUpdateSettingsService.class
         );
-        UpdateSettingsClusterStateUpdateRequest request = new UpdateSettingsClusterStateUpdateRequest().ackTimeout(TimeValue.ZERO);
-        List<Index> indices = new ArrayList<>();
+        List<Index> indicesList = new ArrayList<>();
         for (IndicesService indicesService : internalCluster().getInstances(IndicesService.class)) {
             for (IndexService indexService : indicesService) {
-                indices.add(indexService.index());
+                indicesList.add(indexService.index());
             }
         }
-        request.indices(indices.toArray(Index.EMPTY_ARRAY));
-        request.settings(Settings.builder().put("index.codec", "FastDecompressionCompressingStoredFieldsData").build());
-        request.reopenShards(true);
+        final var indices = indicesList.toArray(Index.EMPTY_ARRAY);
+
+        final Function<Settings.Builder, UpdateSettingsClusterStateUpdateRequest> requestFactory =
+            settings -> new UpdateSettingsClusterStateUpdateRequest(
+                TEST_REQUEST_TIMEOUT,
+                TimeValue.ZERO,
+                settings.build(),
+                UpdateSettingsClusterStateUpdateRequest.OnExisting.OVERWRITE,
+                UpdateSettingsClusterStateUpdateRequest.OnStaticSetting.REOPEN_INDICES,
+                indices
+            );
 
         ClusterService clusterService = internalCluster().getInstance(ClusterService.class);
         AtomicBoolean shardsUnassigned = new AtomicBoolean(false);
@@ -142,47 +163,49 @@ public void testThatNonDynamicSettingChangesDoNotUnncessesarilyCauseReopens() th
 
         AtomicBoolean success = new AtomicBoolean(false);
         // Make the first request, just to set things up:
-        metadataUpdateSettingsService.updateSettings(request, new ActionListener<>() {
-            @Override
-            public void onResponse(AcknowledgedResponse acknowledgedResponse) {
-                success.set(true);
-            }
+        metadataUpdateSettingsService.updateSettings(
+            requestFactory.apply(Settings.builder().put("index.codec", "FastDecompressionCompressingStoredFieldsData")),
+            new ActionListener<>() {
+                @Override
+                public void onResponse(AcknowledgedResponse acknowledgedResponse) {
+                    success.set(true);
+                }
 
-            @Override
-            public void onFailure(Exception e) {
-                fail(e);
+                @Override
+                public void onFailure(Exception e) {
+                    fail(e);
+                }
             }
-        });
+        );
         assertBusy(() -> assertThat(success.get(), equalTo(true)));
         assertBusy(() -> assertThat(expectedSettingsChangeInClusterState.get(), equalTo(true)));
         assertThat(shardsUnassigned.get(), equalTo(true));
 
         assertBusy(() -> assertThat(hasUnassignedShards(clusterService.state(), indexName), equalTo(false)));
 
-        // Same request, except now we'll also set the dynamic "index.max_result_window" setting:
-        request.settings(
-            Settings.builder()
-                .put("index.codec", "FastDecompressionCompressingStoredFieldsData")
-                .put("index.max_result_window", "1500")
-                .build()
-        );
         success.set(false);
         expectedSettingsChangeInClusterState.set(false);
         shardsUnassigned.set(false);
         expectedSetting.set("index.max_result_window");
         expectedSettingValue.set("1500");
         // Making this request ought to add this new setting but not unassign the shards:
-        metadataUpdateSettingsService.updateSettings(request, new ActionListener<>() {
-            @Override
-            public void onResponse(AcknowledgedResponse acknowledgedResponse) {
-                success.set(true);
-            }
+        metadataUpdateSettingsService.updateSettings(
+            // Same request, except now we'll also set the dynamic "index.max_result_window" setting:
+            requestFactory.apply(
+                Settings.builder().put("index.codec", "FastDecompressionCompressingStoredFieldsData").put("index.max_result_window", "1500")
+            ),
+            new ActionListener<>() {
+                @Override
+                public void onResponse(AcknowledgedResponse acknowledgedResponse) {
+                    success.set(true);
+                }
 
-            @Override
-            public void onFailure(Exception e) {
-                fail(e);
+                @Override
+                public void onFailure(Exception e) {
+                    fail(e);
+                }
             }
-        });
+        );
 
         assertBusy(() -> assertThat(success.get(), equalTo(true)));
         assertBusy(() -> assertThat(expectedSettingsChangeInClusterState.get(), equalTo(true)));
diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/settings/put/TransportUpdateSettingsAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/settings/put/TransportUpdateSettingsAction.java
@@ -124,19 +124,24 @@ protected void masterOperation(
             return;
         }
 
-        UpdateSettingsClusterStateUpdateRequest clusterStateUpdateRequest = new UpdateSettingsClusterStateUpdateRequest().indices(
-            concreteIndices
-        )
-            .settings(requestSettings)
-            .setPreserveExisting(request.isPreserveExisting())
-            .reopenShards(request.reopen())
-            .ackTimeout(request.ackTimeout())
-            .masterNodeTimeout(request.masterNodeTimeout());
-
-        updateSettingsService.updateSettings(clusterStateUpdateRequest, listener.delegateResponse((l, e) -> {
-            logger.debug(() -> "failed to update settings on indices [" + Arrays.toString(concreteIndices) + "]", e);
-            l.onFailure(e);
-        }));
+        updateSettingsService.updateSettings(
+            new UpdateSettingsClusterStateUpdateRequest(
+                request.masterNodeTimeout(),
+                request.ackTimeout(),
+                requestSettings,
+                request.isPreserveExisting()
+                    ? UpdateSettingsClusterStateUpdateRequest.OnExisting.PRESERVE
+                    : UpdateSettingsClusterStateUpdateRequest.OnExisting.OVERWRITE,
+                request.reopen()
+                    ? UpdateSettingsClusterStateUpdateRequest.OnStaticSetting.REOPEN_INDICES
+                    : UpdateSettingsClusterStateUpdateRequest.OnStaticSetting.REJECT,
+                concreteIndices
+            ),
+            listener.delegateResponse((l, e) -> {
+                logger.debug(() -> "failed to update settings on indices [" + Arrays.toString(concreteIndices) + "]", e);
+                l.onFailure(e);
+            })
+        );
     }
 
     /**
diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsClusterStateUpdateRequest.java b/server/src/main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsClusterStateUpdateRequest.java
@@ -9,17 +9,23 @@
 
 package org.elasticsearch.action.admin.indices.settings.put;
 
-import org.elasticsearch.cluster.ack.IndicesClusterStateUpdateRequest;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.index.Index;
 
-import java.util.Arrays;
+import java.util.Objects;
 
 /**
  * Cluster state update request that allows to update settings for some indices
  */
-public class UpdateSettingsClusterStateUpdateRequest extends IndicesClusterStateUpdateRequest<UpdateSettingsClusterStateUpdateRequest> {
+public record UpdateSettingsClusterStateUpdateRequest(
+    TimeValue masterNodeTimeout,
+    TimeValue ackTimeout,
+    Settings settings,
+    OnExisting onExisting,
+    OnStaticSetting onStaticSetting,
+    Index... indices
+) {
 
     /**
      * Specifies the behaviour of an update-settings action on existing settings.
@@ -53,79 +59,10 @@ public enum OnStaticSetting {
         REOPEN_INDICES
     }
 
-    private Settings settings;
-
-    private boolean preserveExisting = false;
-
-    private boolean reopenShards = false;
-
-    public UpdateSettingsClusterStateUpdateRequest() {}
-
-    @SuppressWarnings("this-escape")
-    public UpdateSettingsClusterStateUpdateRequest(
-        TimeValue masterNodeTimeout,
-        TimeValue ackTimeout,
-        Settings settings,
-        OnExisting onExisting,
-        OnStaticSetting onStaticSetting,
-        Index... indices
-    ) {
-        masterNodeTimeout(masterNodeTimeout);
-        ackTimeout(ackTimeout);
-        settings(settings);
-        setPreserveExisting(onExisting == OnExisting.PRESERVE);
-        reopenShards(onStaticSetting == OnStaticSetting.REOPEN_INDICES);
-        indices(indices);
-    }
-
-    /**
-     * Returns <code>true</code> iff the settings update should only add but not update settings. If the setting already exists
-     * it should not be overwritten by this update. The default is <code>false</code>
-     */
-    public boolean isPreserveExisting() {
-        return preserveExisting;
-    }
-
-    /**
-     * Returns <code>true</code> if non-dynamic setting updates should go through, by automatically unassigning shards in the same cluster
-     * state change as the setting update. The shards will be automatically reassigned after the cluster state update is made. The
-     * default is <code>false</code>.
-     */
-    public boolean reopenShards() {
-        return reopenShards;
-    }
-
-    public UpdateSettingsClusterStateUpdateRequest reopenShards(boolean reopenShards) {
-        this.reopenShards = reopenShards;
-        return this;
-    }
-
-    /**
-     * Iff set to <code>true</code> this settings update will only add settings not already set on an index. Existing settings remain
-     * unchanged.
-     */
-    public UpdateSettingsClusterStateUpdateRequest setPreserveExisting(boolean preserveExisting) {
-        this.preserveExisting = preserveExisting;
-        return this;
-    }
-
-    /**
-     * Returns the {@link Settings} to update
-     */
-    public Settings settings() {
-        return settings;
-    }
-
-    /**
-     * Sets the {@link Settings} to update
-     */
-    public UpdateSettingsClusterStateUpdateRequest settings(Settings settings) {
-        this.settings = settings;
-        return this;
-    }
-
-    @Override
-    public String toString() {
-        return Arrays.toString(indices()) + settings;
+    public UpdateSettingsClusterStateUpdateRequest {
+        Objects.requireNonNull(masterNodeTimeout);
+        Objects.requireNonNull(ackTimeout);
+        Objects.requireNonNull(settings);
+        Objects.requireNonNull(indices);
     }
 }
diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java
@@ -176,7 +176,7 @@ ClusterState execute(ClusterState currentState) {
             }
             final Settings closedSettings = settingsForClosedIndices.build();
             final Settings openSettings = settingsForOpenIndices.build();
-            final boolean preserveExisting = request.isPreserveExisting();
+            final boolean preserveExisting = request.onExisting() == UpdateSettingsClusterStateUpdateRequest.OnExisting.PRESERVE;
 
             RoutingTable.Builder routingTableBuilder = null;
             Metadata.Builder metadataBuilder = Metadata.builder(currentState.metadata());
@@ -199,7 +199,7 @@ ClusterState execute(ClusterState currentState) {
             }
 
             if (skippedSettings.isEmpty() == false && openIndices.isEmpty() == false) {
-                if (request.reopenShards()) {
+                if (request.onStaticSetting() == UpdateSettingsClusterStateUpdateRequest.OnStaticSetting.REOPEN_INDICES) {
                     // We have non-dynamic settings and open indices. We will unassign all of the shards in these indices so that the new
                     // changed settings are applied when the shards are re-assigned.
                     routingTableBuilder = RoutingTable.builder(
diff --git a/server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java b/server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java
@@ -544,11 +544,18 @@ private CheckedBiConsumer<ActionListener<BulkByScrollResponse>, AcknowledgedResp
      */
     private void setWriteBlock(Index index, boolean readOnlyValue, ActionListener<AcknowledgedResponse> listener) {
         final Settings readOnlySettings = Settings.builder().put(IndexMetadata.INDEX_BLOCKS_WRITE_SETTING.getKey(), readOnlyValue).build();
-        UpdateSettingsClusterStateUpdateRequest updateSettingsRequest = new UpdateSettingsClusterStateUpdateRequest().indices(
-            new Index[] { index }
-        ).settings(readOnlySettings).setPreserveExisting(false).ackTimeout(TimeValue.ZERO);
 
-        metadataUpdateSettingsService.updateSettings(updateSettingsRequest, listener);
+        metadataUpdateSettingsService.updateSettings(
+            new UpdateSettingsClusterStateUpdateRequest(
+                MasterNodeRequest.INFINITE_MASTER_NODE_TIMEOUT,
+                TimeValue.ZERO,
+                readOnlySettings,
+                UpdateSettingsClusterStateUpdateRequest.OnExisting.OVERWRITE,
+                UpdateSettingsClusterStateUpdateRequest.OnStaticSetting.REJECT,
+                index
+            ),
+            listener
+        );
     }
 
     private void reindex(SystemIndexMigrationInfo migrationInfo, ActionListener<BulkByScrollResponse> listener) {
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/settings/TransportUpdateSecuritySettingsAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/settings/TransportUpdateSecuritySettingsAction.java
diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transport/actions/TransportUpdateWatcherSettingsAction.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transport/actions/TransportUpdateWatcherSettingsAction.java
