Force Merge should reject requests with only_expunge_deletes and max_num_segments set

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()) {