Force Merge should reject requests with only_expunge_deletes and max_num_segments set (#44761)

This commit changes the ForceMergeRequest.validate() method so that it does 
not accept the parameters only_expunge_deletes and max_num_segments 
to be set at the same time.

The motivation is that InternalEngine.forceMerge() just ignores the max. number 
of segments parameter when the only expunge parameter is set to true, leaving 
the wrong impression to the user that max. number of segments has been applied. 
It also changes InternalEngine.forceMerge() so that it now throws an exception 
when both parameters are set, and modifies tests where needed.

Because it changes the behavior of the REST API I marked this as >breaking. 

Closes #43102

Author: tlrx

client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java

@@ -1313,6 +1313,10 @@ public void testForceMergeIndex() throws Exception {
             request.onlyExpungeDeletes(true); // <1>
             // end::force-merge-request-only-expunge-deletes
 
+            // set only expunge deletes back to its default value
+            // as it is mutually exclusive with max. num. segments
+            request.onlyExpungeDeletes(ForceMergeRequest.Defaults.ONLY_EXPUNGE_DELETES);
+
             // tag::force-merge-request-flush
             request.flush(true); // <1>
             // end::force-merge-request-flush

docs/reference/migration/migrate_8_0.asciidoc

@@ -27,6 +27,7 @@ coming[8.0.0]
 * <<breaking_80_reindex_changes>>
 * <<breaking_80_search_changes>>
 * <<breaking_80_settings_changes>>
+* <<breaking_80_indices_changes>>
 
 //NOTE: The notable-breaking-changes tagged regions are re-used in the
 //Installation and Upgrade Guide
@@ -65,3 +66,4 @@ include::migrate_8_0/http.asciidoc[]
 include::migrate_8_0/reindex.asciidoc[]
 include::migrate_8_0/search.asciidoc[]
 include::migrate_8_0/settings.asciidoc[]
+include::migrate_8_0/indices.asciidoc[]

docs/reference/migration/migrate_8_0/indices.asciidoc

@@ -0,0 +1,11 @@
+[float]
+[[breaking_80_indices_changes]]
+=== Force Merge API changes
+
+Previously, the Force Merge API allowed the parameters `only_expunge_deletes`
+and `max_num_segments` to be set to a non default value at the same time. But
+the `max_num_segments` was silently ignored when `only_expunge_deletes` is set
+to `true`, leaving the false impression that it has been applied.
+
+The Force Merge API now rejects requests that have a `max_num_segments` greater
+than or equal to 0 when the `only_expunge_deletes` is set to true.

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

@@ -8,3 +8,24 @@
       indices.forcemerge:
         index: testing
         max_num_segments: 1
+
+---
+"Force merge with incompatible only_expunge_deletes and max_num_segments values":
+  - skip:
+      version: " - 7.9.99"
+      reason: only_expunge_deletes and max_num_segments are mutually exclusive since 8.0
+
+  - do:
+      indices.create:
+        index: test
+
+  - do:
+      catch: bad_request
+      indices.forcemerge:
+        index: test
+        max_num_segments: 10
+        only_expunge_deletes: true
+
+  - match: { status: 400 }
+  - match: { error.type: action_request_validation_exception }
+  - match: { error.reason: "Validation Failed: 1: cannot set only_expunge_deletes and max_num_segments at the same time, those two parameters are mutually exclusive;" }

server/src/main/java/org/elasticsearch/action/admin/indices/forcemerge/ForceMergeRequest.java

@@ -19,12 +19,15 @@
 
 package org.elasticsearch.action.admin.indices.forcemerge;
 
+import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.support.broadcast.BroadcastRequest;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 
 import java.io.IOException;
 
+import static org.elasticsearch.action.ValidateActions.addValidationError;
+
 /**
  * A request to force merging the segments of one or more indices. In order to
  * run a merge on all the indices, pass an empty array or {@code null} for the
@@ -122,6 +125,16 @@ public void writeTo(StreamOutput out) throws IOException {
         out.writeBoolean(flush);
     }
 
+    @Override
+    public ActionRequestValidationException validate() {
+        ActionRequestValidationException validationError = super.validate();
+        if (onlyExpungeDeletes && maxNumSegments != Defaults.MAX_NUM_SEGMENTS) {
+            validationError = addValidationError("cannot set only_expunge_deletes and max_num_segments at the same time, those two " +
+                "parameters are mutually exclusive", validationError);
+        }
+        return validationError;
+    }
+
     @Override
     public String toString() {
         return "ForceMergeRequest{" +

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

@@ -1895,6 +1895,9 @@ final Map<BytesRef, VersionValue> getVersionMap() {
     @Override
     public void forceMerge(final boolean flush, int maxNumSegments, boolean onlyExpungeDeletes,
                            final boolean upgrade, final boolean upgradeOnlyAncientSegments) throws EngineException, IOException {
+        if (onlyExpungeDeletes && maxNumSegments >= 0) {
+            throw new IllegalArgumentException("only_expunge_deletes and max_num_segments are mutually exclusive");
+        }
         /*
          * We do NOT acquire the readlock here since we are waiting on the merges to finish
          * that's fine since the IW.rollback should stop all the threads and trigger an IOException

server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/ForceMergeRequestTests.java

@@ -0,0 +1,54 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.elasticsearch.action.admin.indices.forcemerge;
+
+import org.elasticsearch.action.ActionRequestValidationException;
+import org.elasticsearch.test.ESTestCase;
+
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.nullValue;
+
+public class ForceMergeRequestTests extends ESTestCase {
+
+    public void testValidate() {
+        final boolean flush = randomBoolean();
+        final boolean onlyExpungeDeletes = randomBoolean();
+        final int maxNumSegments = randomIntBetween(ForceMergeRequest.Defaults.MAX_NUM_SEGMENTS, 100);
+
+        final ForceMergeRequest request = new ForceMergeRequest();
+        request.flush(flush);
+        request.onlyExpungeDeletes(onlyExpungeDeletes);
+        request.maxNumSegments(maxNumSegments);
+
+        assertThat(request.flush(), equalTo(flush));
+        assertThat(request.onlyExpungeDeletes(), equalTo(onlyExpungeDeletes));
+        assertThat(request.maxNumSegments(), equalTo(maxNumSegments));
+
+        ActionRequestValidationException validation = request.validate();
+        if (onlyExpungeDeletes && maxNumSegments != ForceMergeRequest.Defaults.MAX_NUM_SEGMENTS) {
+            assertThat(validation, notNullValue());
+            assertThat(validation.validationErrors(), contains("cannot set only_expunge_deletes and max_num_segments at the "
+                + "same time, those two parameters are mutually exclusive"));
+        } else {
+            assertThat(validation, nullValue());
+        }
+    }
+}

server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

@@ -189,6 +189,7 @@
 import static org.hamcrest.CoreMatchers.sameInstance;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.everyItem;
@@ -197,6 +198,7 @@
 import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.hasKey;
 import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.isIn;
 import static org.hamcrest.Matchers.lessThanOrEqualTo;
 import static org.hamcrest.Matchers.not;
@@ -1225,8 +1227,7 @@ public void testRenewSyncFlush() throws Exception {
                     Engine.SyncedFlushResult.SUCCESS);
                 assertEquals(3, engine.segments(false).size());
 
-                engine.forceMerge(forceMergeFlushes, 1, false,
-                    false, false);
+                engine.forceMerge(forceMergeFlushes, 1, false, false, false);
                 if (forceMergeFlushes == false) {
                     engine.refresh("make all segments visible");
                     assertEquals(4, engine.segments(false).size());
@@ -1471,7 +1472,7 @@ public void testForceMergeWithoutSoftDeletes() throws IOException {
             Engine.Index index = indexForDoc(doc);
             engine.delete(new Engine.Delete(index.type(), index.id(), index.uid(), primaryTerm.get()));
             //expunge deletes
-            engine.forceMerge(true, 10, true, false, false);
+            engine.forceMerge(true, -1, true, false, false);
             engine.refresh("test");
 
             assertEquals(engine.segments(true).size(), 1);
@@ -1752,8 +1753,7 @@ public void run() {
                                 engine.refresh("test");
                                 indexed.countDown();
                                 try {
-                                    engine.forceMerge(randomBoolean(), 1, false, randomBoolean(),
-                                        randomBoolean());
+                                    engine.forceMerge(randomBoolean(), 1, false, randomBoolean(), randomBoolean());
                                 } catch (IOException e) {
                                     return;
                                 }
@@ -3162,8 +3162,7 @@ public void run() {
                     try {
                         switch (operation) {
                             case "optimize": {
-                                engine.forceMerge(true, 1, false, false,
-                                    false);
+                                engine.forceMerge(true, 1, false, false, false);
                                 break;
                             }
                             case "refresh": {
@@ -4364,7 +4363,16 @@ public void testRandomOperations() throws Exception {
                 engine.flush();
             }
             if (randomBoolean()) {
-                engine.forceMerge(randomBoolean(), between(1, 10), randomBoolean(), false, false);
+                boolean flush = randomBoolean();
+                boolean onlyExpungeDeletes = randomBoolean();
+                int maxNumSegments = randomIntBetween(-1, 10);
+                try {
+                    engine.forceMerge(flush, maxNumSegments, onlyExpungeDeletes, false, false);
+                } catch (IllegalArgumentException e) {
+                    assertThat(e.getMessage(), containsString("only_expunge_deletes and max_num_segments are mutually exclusive"));
+                    assertThat(onlyExpungeDeletes, is(true));
+                    assertThat(maxNumSegments, greaterThan(-1));
+                }
             }
         }
         if (engine.engineConfig.getIndexSettings().isSoftDeleteEnabled()) {