Deprecate not copy settings and explicitly disallow (#30404)

We want copying settings to be the default behavior. This commit
deprecates not copying settings, and disallows explicitly not copying
settings. This gives users a transition path to the future default
behavior.

docs/CHANGELOG.asciidoc

@@ -163,7 +163,8 @@ analysis module.  ({pull}30397[#30397])
 [float]
 === Enhancements
 
-{ref-64}/breaking_64_api_changes.html#copy-source-settings-on-resize[Allow copying source settings on index resize operations] ({pull}30255[#30255])
+{ref-64}/breaking_64_api_changes.html#copy-source-settings-on-resize[Allow
+copying source settings on index resize operations] ({pull}30255[#30255], {pull}30404[#30404])
 
 Added new "Request" object flavored request methods in the RestClient. Prefer
 these instead of the multi-argument versions. ({pull}29623[#29623])

docs/reference/indices/shrink-index.asciidoc

@@ -62,11 +62,20 @@ the following request:
 
 [source,js]
 --------------------------------------------------
-POST my_source_index/_shrink/my_target_index
+POST my_source_index/_shrink/my_target_index?copy_settings=true
+{
+  "settings": {
+    "index.routing.allocation.require._name": null, <1>
+    "index.blocks.write": null <2>
+  }
+}
 --------------------------------------------------
 // CONSOLE
 // TEST[continued]
 
+<1> Clear the allocation requirement copied from the source index.
+<2> Clear the index write block copied from the source index.
+
 The above request returns immediately once the target index has been added to
 the cluster state -- it doesn't wait for the shrink operation to start.
 
@@ -97,7 +106,7 @@ and accepts `settings` and `aliases` parameters for the target index:
 
 [source,js]
 --------------------------------------------------
-POST my_source_index/_shrink/my_target_index
+POST my_source_index/_shrink/my_target_index?copy_settings=true
 {
   "settings": {
     "index.number_of_replicas": 1,
@@ -125,9 +134,11 @@ NOTE: By default, with the exception of `index.analysis`, `index.similarity`,
 and `index.sort` settings, index settings on the source index are not copied
 during a shrink operation. With the exception of non-copyable settings, settings
 from the source index can be copied to the target index by adding the URL
-parameter `copy_settings=true` to the request.
+parameter `copy_settings=true` to the request. Note that `copy_settings` can not
+be set to `false`. The parameter `copy_settings` will be removed in 9.0.0
 
-deprecated[6.4.0, `copy_settings` will default to `true` in 8.x and will be removed in 9.0.0]
+deprecated[6.4.0, not copying settings is deprecated, copying settings will be
+the default behavior in 8.x]
 
 [float]
 === Monitoring the shrink process

docs/reference/indices/split-index.asciidoc

@@ -123,7 +123,7 @@ the following request:
 
 [source,js]
 --------------------------------------------------
-POST my_source_index/_split/my_target_index
+POST my_source_index/_split/my_target_index?copy_settings=true
 {
   "settings": {
     "index.number_of_shards": 2
@@ -158,7 +158,7 @@ and accepts `settings` and `aliases` parameters for the target index:
 
 [source,js]
 --------------------------------------------------
-POST my_source_index/_split/my_target_index
+POST my_source_index/_split/my_target_index?copy_settings=true
 {
   "settings": {
     "index.number_of_shards": 5 <1>
@@ -181,9 +181,11 @@ NOTE: By default, with the exception of `index.analysis`, `index.similarity`,
 and `index.sort` settings, index settings on the source index are not copied
 during a split operation. With the exception of non-copyable settings, settings
 from the source index can be copied to the target index by adding the URL
-parameter `copy_settings=true` to the request.
+parameter `copy_settings=true` to the request. Note that `copy_settings` can not
+be set to `false`. The parameter `copy_settings` will be removed in 9.0.0
 
-deprecated[6.4.0, `copy_settings` will default to `true` in 8.x and will be removed in 9.0.0]
+deprecated[6.4.0, not copying settings is deprecated, copying settings will be
+the default behavior in 8.x]
 
 [float]
 === Monitoring the split process

rest-api-spec/src/main/resources/rest-api-spec/test/indices.shrink/10_basic.yml

@@ -1,5 +1,9 @@
 ---
 "Shrink index via API":
+  - skip:
+      version: " - 6.99.99"
+      reason: expects warnings that pre-7.0.0 will not send
+      features: "warnings"
   # creates an index with one document solely allocated on the master node
   # and shrinks it into a new index with a single shard
   # we don't do the relocation to a single node after the index is created
@@ -62,6 +66,8 @@
         body:
           settings:
             index.number_of_replicas: 0
+      warnings:
+        - "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"
 
   - do:
       cluster.health:

rest-api-spec/src/main/resources/rest-api-spec/test/indices.shrink/20_source_mapping.yml

@@ -1,5 +1,10 @@
 ---
 "Shrink index ignores target template mapping":
+    - skip:
+        version: " - 6.99.99"
+        reason: expects warnings that pre-7.0.0 will not send
+        features: "warnings"
+
     - do:
         cluster.state: {}
         # Get master node id
@@ -65,6 +70,8 @@
           body:
             settings:
               index.number_of_replicas: 0
+        warnings:
+          - "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"
 
     - do:
         cluster.health:

rest-api-spec/src/main/resources/rest-api-spec/test/indices.shrink/30_copy_settings.yml

@@ -1,8 +1,8 @@
 ---
 "Copy settings during shrink index":
   - skip:
-      version: " - 6.3.99"
-      reason:  copy_settings did not exist prior to 6.4.0
+      version: " - 6.99.99"
+      reason: expects warnings that pre-7.0.0 will not send
       features: "warnings"
 
   - do:
@@ -47,8 +47,6 @@
           settings:
             index.number_of_replicas: 0
             index.merge.scheduler.max_thread_count: 2
-      warnings:
-        - "parameter [copy_settings] is deprecated but was [true]"
 
   - do:
       cluster.health:
@@ -64,20 +62,19 @@
   - match: { copy-settings-target.settings.index.blocks.write: "true" }
   - match: { copy-settings-target.settings.index.routing.allocation.include._id: $master }
 
-  # now we do a actual shrink and do not copy settings
+  # now we do a actual shrink and do not copy settings (by default)
   - do:
       indices.shrink:
         index: "source"
         target: "no-copy-settings-target"
         wait_for_active_shards: 1
         master_timeout: 10s
-        copy_settings: false
         body:
           settings:
             index.number_of_replicas: 0
             index.merge.scheduler.max_thread_count: 2
       warnings:
-        - "parameter [copy_settings] is deprecated but was [false]"
+        - "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"
 
   - do:
       cluster.health:
@@ -92,3 +89,16 @@
   - match: { no-copy-settings-target.settings.index.merge.scheduler.max_thread_count: "2" }
   - is_false: no-copy-settings-target.settings.index.blocks.write
   - is_false: no-copy-settings-target.settings.index.routing.allocation.include._id
+
+  # now we do a actual shrink and try to set no copy settings
+  - do:
+      catch: /illegal_argument_exception/
+      indices.shrink:
+        index: "source"
+        target: "explicit-no-copy-settings-target"
+        wait_for_active_shards: 1
+        master_timeout: 10s
+        copy_settings: false
+        body:
+          settings:
+            index.number_of_replicas: 0

rest-api-spec/src/main/resources/rest-api-spec/test/indices.split/10_basic.yml

@@ -33,8 +33,9 @@ setup:
 ---
 "Split index via API":
   - skip:
-      version: " - 6.0.99"
-      reason: Added in 6.1.0
+      version: " - 6.99.99"
+      reason: expects warnings that pre-7.0.0 will not send
+      features: "warnings"
 
   # make it read-only
   - do:
@@ -60,6 +61,8 @@ setup:
           settings:
             index.number_of_replicas: 0
             index.number_of_shards: 4
+      warnings:
+        - "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"
 
   - do:
       cluster.health:
@@ -103,13 +106,13 @@ setup:
 
 ---
 "Split from 1 to N":
-#  - skip:
-#      version: " - 6.99.99"
-#      reason: Added in 7.0.0
-# uncomment once AwaitsFix is resolved
   - skip:
+      # when re-enabling uncomment the below skips
       version: "all"
       reason: "AwaitsFix'ing, see https://github.com/elastic/elasticsearch/issues/30503"
+      # version: " - 6.99.99"
+      # reason: expects warnings that pre-7.0.0 will not send
+      features: "warnings"
   - do:
       indices.create:
         index: source_one_shard
@@ -163,6 +166,8 @@ setup:
           settings:
             index.number_of_replicas: 0
             index.number_of_shards: 5
+      warnings:
+        - "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"
 
   - do:
       cluster.health:
@@ -208,8 +213,9 @@ setup:
 ---
 "Create illegal split indices":
   - skip:
-      version: " - 6.0.99"
-      reason: Added in 6.1.0
+      version: " - 6.99.99"
+      reason: expects warnings that pre-7.0.0 will not send
+      features: "warnings"
 
   # try to do an illegal split with number_of_routing_shards set
   - do:
@@ -224,6 +230,8 @@ setup:
             index.number_of_replicas: 0
             index.number_of_shards: 4
             index.number_of_routing_shards: 8
+      warnings:
+        - "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"
 
   # try to do an illegal split with illegal number_of_shards
   - do:
@@ -237,3 +245,5 @@ setup:
           settings:
             index.number_of_replicas: 0
             index.number_of_shards: 6
+      warnings:
+        - "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"

rest-api-spec/src/main/resources/rest-api-spec/test/indices.split/20_source_mapping.yml

@@ -1,12 +1,12 @@
 ---
 "Split index ignores target template mapping":
-#  - skip:
-#      version: " - 6.0.99"
-#      reason: Added in 6.1.0
-# uncomment once AwaitsFix is resolved
   - skip:
+      # when re-enabling uncomment the below skips
       version: "all"
       reason: "AwaitsFix'ing, see https://github.com/elastic/elasticsearch/issues/30503"
+      # version: " - 6.99.99"
+      # reason: expects warnings that pre-7.0.0 will not send
+      features: "warnings"
 
   # create index
   - do:
@@ -68,6 +68,8 @@
           settings:
             index.number_of_shards: 2
             index.number_of_replicas: 0
+      warnings:
+        - "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"
 
   - do:
       cluster.health:

rest-api-spec/src/main/resources/rest-api-spec/test/indices.split/30_copy_settings.yml

@@ -1,8 +1,8 @@
 ---
 "Copy settings during split index":
   - skip:
-      version: " - 6.3.99"
-      reason:  copy_settings did not exist prior to 6.4.0
+      version: " - 6.99.99"
+      reason: expects warnings that pre-7.0.0 will not send
       features: "warnings"
 
   - do:
@@ -50,8 +50,6 @@
             index.number_of_replicas: 0
             index.number_of_shards: 2
             index.merge.scheduler.max_thread_count: 2
-      warnings:
-        - "parameter [copy_settings] is deprecated but was [true]"
 
   - do:
       cluster.health:
@@ -67,21 +65,20 @@
   - match: { copy-settings-target.settings.index.blocks.write: "true" }
   - match: { copy-settings-target.settings.index.routing.allocation.include._id: $master }
 
-  # now we do a actual shrink and do not copy settings
+  # now we do a actual shrink and do not copy settings (by default)
   - do:
       indices.split:
         index: "source"
         target: "no-copy-settings-target"
         wait_for_active_shards: 1
         master_timeout: 10s
-        copy_settings: false
         body:
           settings:
             index.number_of_replicas: 0
             index.number_of_shards: 2
             index.merge.scheduler.max_thread_count: 2
       warnings:
-        - "parameter [copy_settings] is deprecated but was [false]"
+        - "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"
 
   - do:
       cluster.health:
@@ -96,3 +93,15 @@
   - match: { no-copy-settings-target.settings.index.merge.scheduler.max_thread_count: "2" }
   - is_false: no-copy-settings-target.settings.index.blocks.write
   - is_false: no-copy-settings-target.settings.index.routing.allocation.include._id
+
+  - do:
+      catch: /illegal_argument_exception/
+      indices.split:
+        index: "source"
+        target: "explicit-no-copy-settings-target"
+        wait_for_active_shards: 1
+        master_timeout: 10s
+        copy_settings: false
+        body:
+          settings:
+            index.number_of_replicas: 0

server/src/main/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequest.java

@@ -56,7 +56,7 @@ public class ResizeRequest extends AcknowledgedRequest<ResizeRequest> implements
     private CreateIndexRequest targetIndexRequest;
     private String sourceIndex;
     private ResizeType type = ResizeType.SHRINK;
-    private boolean copySettings = false;
+    private Boolean copySettings;
 
     ResizeRequest() {}
 
@@ -80,6 +80,7 @@ public class ResizeRequest extends AcknowledgedRequest<ResizeRequest> implements
         if (type == ResizeType.SPLIT && IndexMetaData.INDEX_NUMBER_OF_SHARDS_SETTING.exists(targetIndexRequest.settings()) == false) {
             validationException = addValidationError("index.number_of_shards is required for split operations", validationException);
         }
+        assert copySettings == null || copySettings;
         return validationException;
     }
 
@@ -98,10 +99,12 @@ public class ResizeRequest extends AcknowledgedRequest<ResizeRequest> implements
         } else {
             type = ResizeType.SHRINK; // BWC this used to be shrink only
         }
-        if (in.getVersion().onOrAfter(Version.V_6_4_0)) {
+        if (in.getVersion().before(Version.V_6_4_0)) {
+            copySettings = null;
+        } else if (in.getVersion().onOrAfter(Version.V_6_4_0) && in.getVersion().before(Version.V_7_0_0_alpha1)){
             copySettings = in.readBoolean();
         } else {
-            copySettings = false;
+            copySettings = in.readOptionalBoolean();
         }
     }
 
@@ -113,8 +116,12 @@ public class ResizeRequest extends AcknowledgedRequest<ResizeRequest> implements
         if (out.getVersion().onOrAfter(ResizeAction.COMPATIBILITY_VERSION)) {
             out.writeEnum(type);
         }
-        if (out.getVersion().onOrAfter(Version.V_6_4_0)) {
-            out.writeBoolean(copySettings);
+        if (out.getVersion().before(Version.V_6_4_0)) {
+
+        } else if (out.getVersion().onOrAfter(Version.V_6_4_0) && out.getVersion().before(Version.V_7_0_0_alpha1)) {
+            out.writeBoolean(copySettings == null ? false : copySettings);
+        } else {
+            out.writeOptionalBoolean(copySettings);
         }
     }
 
@@ -187,11 +194,14 @@ public class ResizeRequest extends AcknowledgedRequest<ResizeRequest> implements
         return type;
     }
 
-    public void setCopySettings(final boolean copySettings) {
+    public void setCopySettings(final Boolean copySettings) {
+        if (copySettings != null && copySettings == false) {
+            throw new IllegalArgumentException("[copySettings] can not be explicitly set to [false]");
+        }
         this.copySettings = copySettings;
     }
 
-    public boolean getCopySettings() {
+    public Boolean getCopySettings() {
         return copySettings;
     }
 

server/src/main/java/org/elasticsearch/action/admin/indices/shrink/TransportResizeAction.java

@@ -190,7 +190,7 @@ public class TransportResizeAction extends TransportMasterNodeAction<ResizeReque
                 .waitForActiveShards(targetIndex.waitForActiveShards())
                 .recoverFrom(metaData.getIndex())
                 .resizeType(resizeRequest.getResizeType())
-                .copySettings(resizeRequest.getCopySettings());
+                .copySettings(resizeRequest.getCopySettings() == null ? false : resizeRequest.getCopySettings());
     }
 
     @Override

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestResizeHandler.java

@@ -48,17 +48,22 @@ public abstract class RestResizeHandler extends BaseRestHandler {
         final ResizeRequest resizeRequest = new ResizeRequest(request.param("target"), request.param("index"));
         resizeRequest.setResizeType(getResizeType());
         final String rawCopySettings = request.param("copy_settings");
-        final boolean copySettings;
+        final Boolean copySettings;
         if (rawCopySettings == null) {
             copySettings = resizeRequest.getCopySettings();
+        } else if (rawCopySettings.isEmpty()) {
+            copySettings = true;
         } else {
-            deprecationLogger.deprecated("parameter [copy_settings] is deprecated but was [" + rawCopySettings + "]");
-            if (rawCopySettings.length() == 0) {
-                copySettings = true;
-            } else {
-                copySettings = Booleans.parseBoolean(rawCopySettings);
+            copySettings = Booleans.parseBoolean(rawCopySettings);
+            if (copySettings == false) {
+                throw new IllegalArgumentException("parameter [copy_settings] can not be explicitly set to [false]");
             }
         }
+        if (copySettings == null) {
+            deprecationLogger.deprecated(
+                    "resize operations without copying settings is deprecated; "
+                            + "set parameter [copy_settings] to [true] for future default behavior");
+        }
         resizeRequest.setCopySettings(copySettings);
         request.applyContentParser(resizeRequest::fromXContent);
         resizeRequest.timeout(request.paramAsTime("timeout", resizeRequest.timeout()));

server/src/test/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequestTests.java

@@ -31,12 +31,34 @@ import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.hamcrest.ElasticsearchAssertions;
 
 import java.io.IOException;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
 
 import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_SHARDS;
 import static org.elasticsearch.common.xcontent.ToXContent.EMPTY_PARAMS;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.hasToString;
 
 public class ResizeRequestTests extends ESTestCase {
 
+    public void testCopySettingsValidation() {
+        runTestCopySettingsValidation(false, r -> {
+            final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, r::get);
+            assertThat(e, hasToString(containsString("[copySettings] can not be explicitly set to [false]")));
+        });
+
+        runTestCopySettingsValidation(null, r -> assertNull(r.get().getCopySettings()));
+        runTestCopySettingsValidation(true, r -> assertTrue(r.get().getCopySettings()));
+    }
+
+    private void runTestCopySettingsValidation(final Boolean copySettings, final Consumer<Supplier<ResizeRequest>> consumer) {
+        consumer.accept(() -> {
+            final ResizeRequest request = new ResizeRequest();
+            request.setCopySettings(copySettings);
+            return request;
+        });
+    }
+
     public void testToXContent() throws IOException {
         {
             ResizeRequest request = new ResizeRequest("target", "source");

server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestResizeHandlerTests.java

@@ -20,15 +20,20 @@
 package org.elasticsearch.rest.action.admin.indices;
 
 import org.elasticsearch.client.node.NodeClient;
+import org.elasticsearch.common.Booleans;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.xcontent.NamedXContentRegistry;
 import org.elasticsearch.rest.RestController;
+import org.elasticsearch.rest.RestHandler;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.rest.FakeRestRequest;
 
 import java.io.IOException;
 import java.util.Collections;
+import java.util.Locale;
 
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.hasToString;
 import static org.mockito.Mockito.mock;
 
 public class RestResizeHandlerTests extends ESTestCase {
@@ -36,27 +41,41 @@ public class RestResizeHandlerTests extends ESTestCase {
     public void testShrinkCopySettingsDeprecated() throws IOException {
         final RestResizeHandler.RestShrinkIndexAction handler =
                 new RestResizeHandler.RestShrinkIndexAction(Settings.EMPTY, mock(RestController.class));
-        final String copySettings = randomFrom("true", "false");
-        final FakeRestRequest request =
-                new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
-                        .withParams(Collections.singletonMap("copy_settings", copySettings))
-                        .withPath("source/_shrink/target")
-                        .build();
-        handler.prepareRequest(request, mock(NodeClient.class));
-        assertWarnings("parameter [copy_settings] is deprecated but was [" + copySettings + "]");
+        for (final String copySettings : new String[]{null, "", "true", "false"}) {
+            runTestResizeCopySettingsDeprecated(handler, "shrink", copySettings);
+        }
     }
 
     public void testSplitCopySettingsDeprecated() throws IOException {
         final RestResizeHandler.RestSplitIndexAction handler =
                 new RestResizeHandler.RestSplitIndexAction(Settings.EMPTY, mock(RestController.class));
-        final String copySettings = randomFrom("true", "false");
-        final FakeRestRequest request =
+        for (final String copySettings : new String[]{null, "", "true", "false"}) {
+            runTestResizeCopySettingsDeprecated(handler, "split", copySettings);
+        }
+    }
+
+    private void runTestResizeCopySettingsDeprecated(
+            final RestResizeHandler handler, final String resizeOperation, final String copySettings) throws IOException {
+        final FakeRestRequest.Builder builder =
                 new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
                         .withParams(Collections.singletonMap("copy_settings", copySettings))
-                        .withPath("source/_split/target")
-                        .build();
-        handler.prepareRequest(request, mock(NodeClient.class));
-        assertWarnings("parameter [copy_settings] is deprecated but was [" + copySettings + "]");
+                        .withPath(String.format(Locale.ROOT, "source/_%s/target", resizeOperation));
+        if (copySettings != null) {
+            builder.withParams(Collections.singletonMap("copy_settings", copySettings));
+        }
+        final FakeRestRequest request = builder.build();
+        if ("false".equals(copySettings)) {
+            final IllegalArgumentException e =
+                    expectThrows(IllegalArgumentException.class, () -> handler.prepareRequest(request, mock(NodeClient.class)));
+            assertThat(e, hasToString(containsString("parameter [copy_settings] can not be explicitly set to [false]")));
+        } else {
+            handler.prepareRequest(request, mock(NodeClient.class));
+            if (copySettings == null) {
+                assertWarnings(
+                        "resize operations without copying settings is deprecated; "
+                                + "set parameter [copy_settings] to [true] for future default behavior");
+            }
+        }
     }
 
 }