Forbid null ack timeout (elastic#107653)

Today a cluster state update task with `null` ack timeout implies the timeout is zero. This implicit behaviour is trappy, so this commit forbids it. Relates elastic#107044
afoucret · Apr 22, 2024 · c0417b3 · c0417b3
1 parent 2505655
commit c0417b3
Show file tree

Hide file tree

Showing 3 changed files with 8 additions and 3 deletions.
diff --git a/...lClusterTest/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsServiceIT.java b/...lClusterTest/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsServiceIT.java
@@ -15,6 +15,7 @@
 import org.elasticsearch.cluster.routing.ShardRoutingState;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.index.Index;
 import org.elasticsearch.index.IndexService;
 import org.elasticsearch.index.engine.Engine;
@@ -40,7 +41,7 @@ public void testThatNonDynamicSettingChangesTakeEffect() throws Exception {
         MetadataUpdateSettingsService metadataUpdateSettingsService = internalCluster().getCurrentMasterNodeInstance(
             MetadataUpdateSettingsService.class
         );
-        UpdateSettingsClusterStateUpdateRequest request = new UpdateSettingsClusterStateUpdateRequest();
+        UpdateSettingsClusterStateUpdateRequest request = new UpdateSettingsClusterStateUpdateRequest().ackTimeout(TimeValue.ZERO);
         List<Index> indices = new ArrayList<>();
         for (IndicesService indicesService : internalCluster().getInstances(IndicesService.class)) {
             for (IndexService indexService : indicesService) {
@@ -108,7 +109,7 @@ public void testThatNonDynamicSettingChangesDoNotUnncessesarilyCauseReopens() th
         MetadataUpdateSettingsService metadataUpdateSettingsService = internalCluster().getCurrentMasterNodeInstance(
             MetadataUpdateSettingsService.class
         );
-        UpdateSettingsClusterStateUpdateRequest request = new UpdateSettingsClusterStateUpdateRequest();
+        UpdateSettingsClusterStateUpdateRequest request = new UpdateSettingsClusterStateUpdateRequest().ackTimeout(TimeValue.ZERO);
         List<Index> indices = new ArrayList<>();
         for (IndicesService indicesService : internalCluster().getInstances(IndicesService.class)) {
             for (IndexService indexService : indicesService) {

diff --git a/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java b/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java
@@ -46,6 +46,7 @@
 import org.elasticsearch.core.Releasables;
 import org.elasticsearch.core.SuppressForbidden;
 import org.elasticsearch.core.TimeValue;
+import org.elasticsearch.core.UpdateForV9;
 import org.elasticsearch.node.Node;
 import org.elasticsearch.tasks.Task;
 import org.elasticsearch.tasks.TaskAwareRequest;
@@ -703,9 +704,11 @@ private static class TaskAckListener {
             this.countDown = new CountDown(countDown + 1); // we also wait for onCommit to be called
         }
 
+        @UpdateForV9 // properly forbid ackTimeout == null after enough time has passed to be sure it's not used in production
         public void onCommit(TimeValue commitTime) {
             TimeValue ackTimeout = contextPreservingAckListener.ackTimeout();
             if (ackTimeout == null) {
+                assert false : "ackTimeout must always be present: " + contextPreservingAckListener;
                 ackTimeout = TimeValue.ZERO;
             }
             final TimeValue timeLeft = TimeValue.timeValueNanos(Math.max(0, ackTimeout.nanos() - commitTime.nanos()));

diff --git a/server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java b/server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java
@@ -35,6 +35,7 @@
 import org.elasticsearch.common.settings.IndexScopedSettings;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.core.SuppressForbidden;
+import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.index.Index;
 import org.elasticsearch.index.IndexNotFoundException;
 import org.elasticsearch.index.reindex.BulkByScrollResponse;
@@ -537,7 +538,7 @@ private void setWriteBlock(Index index, boolean readOnlyValue, ActionListener<Ac
         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);
+        ).settings(readOnlySettings).setPreserveExisting(false).ackTimeout(TimeValue.ZERO);
 
         metadataUpdateSettingsService.updateSettings(updateSettingsRequest, listener);
     }