From 3fa06058cc78880ece5f45ee4912537002606418 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Tue, 23 Jul 2019 16:36:20 +0200 Subject: [PATCH 1/5] Force Merge should barf when only_expunge_deletes and max_num_segments are both set --- .../test/indices.forcemerge/10_basic.yml | 21 +++++++++++ .../indices/forcemerge/ForceMergeRequest.java | 13 +++++++ .../index/engine/InternalEngine.java | 3 ++ .../forcemerge/ForceMergeRequestTests.java | 36 +++++++++++++++++++ .../index/engine/InternalEngineTests.java | 24 ++++++++----- 5 files changed, 89 insertions(+), 8 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/ForceMergeRequestTests.java diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.forcemerge/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.forcemerge/10_basic.yml index 6f1c6ea949665..0889effc3d509 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.forcemerge/10_basic.yml +++ b/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;" } diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/forcemerge/ForceMergeRequest.java b/server/src/main/java/org/elasticsearch/action/admin/indices/forcemerge/ForceMergeRequest.java index b7fa9094540a7..bc810d4f6477d 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/forcemerge/ForceMergeRequest.java +++ b/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{" + diff --git a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java index af0adfdedcf45..5ea51c57f90b9 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java +++ b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java @@ -1895,6 +1895,9 @@ final Map 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 diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/ForceMergeRequestTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/ForceMergeRequestTests.java new file mode 100644 index 0000000000000..2e9d1dd3332c4 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/ForceMergeRequestTests.java @@ -0,0 +1,36 @@ +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()); + } + } +} diff --git a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java b/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java index 0df178f924e58..8b56c0181adfb 100644 --- a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java +++ b/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()) { From 9b8f6964f8577498bc540dd86a29a55619435f29 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Tue, 23 Jul 2019 16:59:16 +0200 Subject: [PATCH 2/5] Add breaking notes --- docs/reference/migration/migrate_8_0.asciidoc | 1 + docs/reference/migration/migrate_8_0/indices.asciidoc | 11 +++++++++++ 2 files changed, 12 insertions(+) create mode 100644 docs/reference/migration/migrate_8_0/indices.asciidoc diff --git a/docs/reference/migration/migrate_8_0.asciidoc b/docs/reference/migration/migrate_8_0.asciidoc index ff3f5030ed9fb..3c535b9a1306a 100644 --- a/docs/reference/migration/migrate_8_0.asciidoc +++ b/docs/reference/migration/migrate_8_0.asciidoc @@ -27,6 +27,7 @@ coming[8.0.0] * <> * <> * <> +* <> //NOTE: The notable-breaking-changes tagged regions are re-used in the //Installation and Upgrade Guide diff --git a/docs/reference/migration/migrate_8_0/indices.asciidoc b/docs/reference/migration/migrate_8_0/indices.asciidoc new file mode 100644 index 0000000000000..05b9a299b9ec1 --- /dev/null +++ b/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. From ccdad777fadf37e3a76fc55599fbf490c366d90f Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Tue, 23 Jul 2019 17:30:25 +0200 Subject: [PATCH 3/5] fix license header --- .../forcemerge/ForceMergeRequestTests.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/ForceMergeRequestTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/ForceMergeRequestTests.java index 2e9d1dd3332c4..f672a22aaf509 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/ForceMergeRequestTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/ForceMergeRequestTests.java @@ -1,3 +1,21 @@ +/* + * 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; From 6fd21913189ae4232fde3baf84f402419f9dc4bd Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Tue, 23 Jul 2019 17:46:10 +0200 Subject: [PATCH 4/5] add missing include docs --- docs/reference/migration/migrate_8_0.asciidoc | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/reference/migration/migrate_8_0.asciidoc b/docs/reference/migration/migrate_8_0.asciidoc index 3c535b9a1306a..4f56b628caf1a 100644 --- a/docs/reference/migration/migrate_8_0.asciidoc +++ b/docs/reference/migration/migrate_8_0.asciidoc @@ -66,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[] From 85980d0021fce1578e08798962070ae75d63a05a Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Wed, 24 Jul 2019 09:48:46 +0200 Subject: [PATCH 5/5] Fix IndicesClientDocumentationIT.java --- .../client/documentation/IndicesClientDocumentationIT.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java index f878f0f6f7d88..4a0f93a5c86da 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java @@ -1312,6 +1312,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