From 3daefe66af6f6246d4c9fb625d65d75ffc76cd4a Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Wed, 25 Jul 2018 14:11:14 -0600 Subject: [PATCH] Remove "best_compression" option from the ForceMergeAction (#32373) This option is only settable while the index is closed, and doesn't make sense for a force merge. Relates to #29823 --- .../core/indexlifecycle/ForceMergeAction.java | 33 +++---------------- .../core/indexlifecycle/SegmentCountStep.java | 29 +++------------- .../indexlifecycle/ForceMergeActionTests.java | 25 +++----------- .../indexlifecycle/SegmentCountStepTests.java | 23 ++++--------- .../TimeseriesLifecycleTypeTests.java | 4 +-- .../IndexLifecycleInitialisationIT.java | 2 +- .../TimeSeriesLifecycleActionsIT.java | 2 +- 7 files changed, 25 insertions(+), 93 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/ForceMergeAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/ForceMergeAction.java index 5aea7a98521..5fedb14ecff 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/ForceMergeAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/ForceMergeAction.java @@ -10,12 +10,9 @@ import org.elasticsearch.common.ParseField; import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.index.codec.CodecService; -import org.elasticsearch.index.engine.EngineConfig; import org.elasticsearch.xpack.core.indexlifecycle.Step.StepKey; import java.io.IOException; @@ -29,53 +26,42 @@ import java.util.Objects; public class ForceMergeAction implements LifecycleAction { public static final String NAME = "forcemerge"; public static final ParseField MAX_NUM_SEGMENTS_FIELD = new ParseField("max_num_segments"); - public static final ParseField BEST_COMPRESSION_FIELD = new ParseField("best_compression"); private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>(NAME, false, a -> { int maxNumSegments = (int) a[0]; - boolean bestCompression = a[1] == null ? false : (boolean) a[1]; - return new ForceMergeAction(maxNumSegments, bestCompression); + return new ForceMergeAction(maxNumSegments); }); static { PARSER.declareInt(ConstructingObjectParser.constructorArg(), MAX_NUM_SEGMENTS_FIELD); - PARSER.declareBoolean(ConstructingObjectParser.optionalConstructorArg(), BEST_COMPRESSION_FIELD); } private final int maxNumSegments; - private final boolean bestCompression; public static ForceMergeAction parse(XContentParser parser) { return PARSER.apply(parser, null); } - public ForceMergeAction(int maxNumSegments, boolean bestCompression) { + public ForceMergeAction(int maxNumSegments) { if (maxNumSegments <= 0) { throw new IllegalArgumentException("[" + MAX_NUM_SEGMENTS_FIELD.getPreferredName() + "] must be a positive integer"); } this.maxNumSegments = maxNumSegments; - this.bestCompression = bestCompression; } public ForceMergeAction(StreamInput in) throws IOException { this.maxNumSegments = in.readVInt(); - this.bestCompression = in.readBoolean(); } public int getMaxNumSegments() { return maxNumSegments; } - public boolean isBestCompression() { - return bestCompression; - } - @Override public void writeTo(StreamOutput out) throws IOException { out.writeVInt(maxNumSegments); - out.writeBoolean(bestCompression); } @Override @@ -92,7 +78,6 @@ public class ForceMergeAction implements LifecycleAction { public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); builder.field(MAX_NUM_SEGMENTS_FIELD.getPreferredName(), maxNumSegments); - builder.field(BEST_COMPRESSION_FIELD.getPreferredName(), bestCompression); builder.endObject(); return builder; } @@ -103,20 +88,13 @@ public class ForceMergeAction implements LifecycleAction { StepKey forceMergeKey = new StepKey(phase, NAME, ForceMergeStep.NAME); StepKey countKey = new StepKey(phase, NAME, SegmentCountStep.NAME); ForceMergeStep forceMergeStep = new ForceMergeStep(forceMergeKey, countKey, client, maxNumSegments); - SegmentCountStep segmentCountStep = new SegmentCountStep(countKey, nextStepKey, client, maxNumSegments, bestCompression); - if (bestCompression) { - Settings compressionSettings = Settings.builder() - .put(EngineConfig.INDEX_CODEC_SETTING.getKey(), CodecService.BEST_COMPRESSION_CODEC).build(); - UpdateSettingsStep updateBestCompression = new UpdateSettingsStep(updateCompressionKey, - forceMergeKey, client, compressionSettings); - return Arrays.asList(updateBestCompression, forceMergeStep, segmentCountStep); - } + SegmentCountStep segmentCountStep = new SegmentCountStep(countKey, nextStepKey, client, maxNumSegments); return Arrays.asList(forceMergeStep, segmentCountStep); } @Override public int hashCode() { - return Objects.hash(maxNumSegments, bestCompression); + return Objects.hash(maxNumSegments); } @Override @@ -128,8 +106,7 @@ public class ForceMergeAction implements LifecycleAction { return false; } ForceMergeAction other = (ForceMergeAction) obj; - return Objects.equals(maxNumSegments, other.maxNumSegments) - && Objects.equals(bestCompression, other.bestCompression); + return Objects.equals(maxNumSegments, other.maxNumSegments); } @Override diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/SegmentCountStep.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/SegmentCountStep.java index 7b07e674905..f775022cf85 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/SegmentCountStep.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/SegmentCountStep.java @@ -27,48 +27,28 @@ public class SegmentCountStep extends AsyncWaitStep { public static final String NAME = "segment-count"; private final int maxNumSegments; - private final boolean bestCompression; - public SegmentCountStep(StepKey key, StepKey nextStepKey, Client client, int maxNumSegments, boolean bestCompression) { + public SegmentCountStep(StepKey key, StepKey nextStepKey, Client client, int maxNumSegments) { super(key, nextStepKey, client); this.maxNumSegments = maxNumSegments; - this.bestCompression = bestCompression; } public int getMaxNumSegments() { return maxNumSegments; } - public boolean isBestCompression() { - return bestCompression; - } - @Override public void evaluateCondition(Index index, Listener listener) { getClient().admin().indices().segments(new IndicesSegmentsRequest(index.getName()), ActionListener.wrap(response -> { long numberShardsLeftToMerge = StreamSupport.stream(response.getIndices().get(index.getName()).spliterator(), false) - .filter(iss -> Arrays.stream(iss.getShards()).anyMatch(p -> { - boolean hasRightAmountOfSegments = p.getSegments().size() <= maxNumSegments; - if (bestCompression) { -// // TODO(talevy): discuss -// boolean allUsingCorrectCompression = p.getSegments().stream().anyMatch(s -> -// Lucene50StoredFieldsFormat.Mode.BEST_COMPRESSION.equals( -// Lucene50StoredFieldsFormat.Mode.BEST_COMPRESSION.toString().equals( -// s.getAttributes().get(Lucene50StoredFieldsFormat.MODE_KEY))) -// ); - boolean allUsingCorrectCompression = true; - return (hasRightAmountOfSegments && allUsingCorrectCompression) == false; - } else { - return hasRightAmountOfSegments == false; - } - })).count(); + .filter(iss -> Arrays.stream(iss.getShards()).anyMatch(p -> p.getSegments().size() > maxNumSegments)).count(); listener.onResponse(numberShardsLeftToMerge == 0, new Info(numberShardsLeftToMerge)); }, listener::onFailure)); } @Override public int hashCode() { - return Objects.hash(super.hashCode(), maxNumSegments, bestCompression); + return Objects.hash(super.hashCode(), maxNumSegments); } @Override @@ -81,8 +61,7 @@ public class SegmentCountStep extends AsyncWaitStep { } SegmentCountStep other = (SegmentCountStep) obj; return super.equals(obj) - && Objects.equals(maxNumSegments, other.maxNumSegments) - && Objects.equals(bestCompression, other.bestCompression); + && Objects.equals(maxNumSegments, other.maxNumSegments); } public static class Info implements ToXContentObject { diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/ForceMergeActionTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/ForceMergeActionTests.java index 16040833d9e..a1bf6ec0984 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/ForceMergeActionTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/ForceMergeActionTests.java @@ -7,7 +7,6 @@ package org.elasticsearch.xpack.core.indexlifecycle; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.Writeable.Reader; -import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.DeprecationHandler; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.XContentParser; @@ -29,19 +28,14 @@ public class ForceMergeActionTests extends AbstractActionTestCase new ForceMergeAction(randomIntBetween(-10, 0), false)); + Exception r = expectThrows(IllegalArgumentException.class, () -> new ForceMergeAction(randomIntBetween(-10, 0))); assertThat(r.getMessage(), equalTo("[max_num_segments] must be a positive integer")); } @@ -69,16 +63,7 @@ public class ForceMergeActionTests extends AbstractActionTestCase steps = instance.toSteps(null, phase, nextStepKey); assertNotNull(steps); int nextFirstIndex = 0; - if (instance.isBestCompression()) { - Settings expectedSettings = Settings.builder().put("index.codec", "best_compression").build(); - assertEquals(3, steps.size()); - UpdateSettingsStep firstStep = (UpdateSettingsStep) steps.get(0); - assertThat(firstStep.getKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, "best_compression"))); - assertThat(firstStep.getSettings(), equalTo(expectedSettings)); - nextFirstIndex = 1; - } else { - assertEquals(2, steps.size()); - } + assertEquals(2, steps.size()); ForceMergeStep firstStep = (ForceMergeStep) steps.get(nextFirstIndex); SegmentCountStep secondStep = (SegmentCountStep) steps.get(nextFirstIndex + 1); assertThat(firstStep.getKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, ForceMergeStep.NAME))); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/SegmentCountStepTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/SegmentCountStepTests.java index 829bf66549a..faa63feeed1 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/SegmentCountStepTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/SegmentCountStepTests.java @@ -37,9 +37,8 @@ public class SegmentCountStepTests extends AbstractStepTestCase { @SuppressWarnings("unchecked") @@ -115,7 +108,7 @@ public class SegmentCountStepTests extends AbstractStepTestCase conditionMetResult = new SetOnce<>(); SetOnce conditionInfo = new SetOnce<>(); - SegmentCountStep step = new SegmentCountStep(stepKey, nextStepKey, client, maxNumSegments, bestCompression); + SegmentCountStep step = new SegmentCountStep(stepKey, nextStepKey, client, maxNumSegments); step.evaluateCondition(index, new AsyncWaitStep.Listener() { @Override public void onResponse(boolean conditionMet, ToXContentObject info) { @@ -161,7 +154,6 @@ public class SegmentCountStepTests extends AbstractStepTestCase { @SuppressWarnings("unchecked") @@ -173,7 +165,7 @@ public class SegmentCountStepTests extends AbstractStepTestCase conditionMetResult = new SetOnce<>(); SetOnce conditionInfo = new SetOnce<>(); - SegmentCountStep step = new SegmentCountStep(stepKey, nextStepKey, client, maxNumSegments, bestCompression); + SegmentCountStep step = new SegmentCountStep(stepKey, nextStepKey, client, maxNumSegments); step.evaluateCondition(index, new AsyncWaitStep.Listener() { @Override public void onResponse(boolean conditionMet, ToXContentObject info) { @@ -203,7 +195,6 @@ public class SegmentCountStepTests extends AbstractStepTestCase { @SuppressWarnings("unchecked") @@ -214,7 +205,7 @@ public class SegmentCountStepTests extends AbstractStepTestCase exceptionThrown = new SetOnce<>(); - SegmentCountStep step = new SegmentCountStep(stepKey, nextStepKey, client, maxNumSegments, bestCompression); + SegmentCountStep step = new SegmentCountStep(stepKey, nextStepKey, client, maxNumSegments); step.evaluateCondition(index, new AsyncWaitStep.Listener() { @Override public void onResponse(boolean conditionMet, ToXContentObject info) { diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/TimeseriesLifecycleTypeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/TimeseriesLifecycleTypeTests.java index 6dca250a4bc..c54efd38fcd 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/TimeseriesLifecycleTypeTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/TimeseriesLifecycleTypeTests.java @@ -28,10 +28,10 @@ import static org.elasticsearch.xpack.core.indexlifecycle.TimeseriesLifecycleTyp import static org.hamcrest.Matchers.equalTo; public class TimeseriesLifecycleTypeTests extends ESTestCase { - + private static final AllocateAction TEST_ALLOCATE_ACTION = new AllocateAction(Collections.singletonMap("node", "node1"),null, null); private static final DeleteAction TEST_DELETE_ACTION = new DeleteAction(); - private static final ForceMergeAction TEST_FORCE_MERGE_ACTION = new ForceMergeAction(1, true); + private static final ForceMergeAction TEST_FORCE_MERGE_ACTION = new ForceMergeAction(1); private static final ReplicasAction TEST_REPLICAS_ACTION = new ReplicasAction(1); private static final RolloverAction TEST_ROLLOVER_ACTION = new RolloverAction(new ByteSizeValue(1), null, null); private static final ShrinkAction TEST_SHRINK_ACTION = new ShrinkAction(1); diff --git a/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleInitialisationIT.java b/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleInitialisationIT.java index 8f01c801046..248a2b317e5 100644 --- a/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleInitialisationIT.java +++ b/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleInitialisationIT.java @@ -97,7 +97,7 @@ public class IndexLifecycleInitialisationIT extends ESIntegTestCase { .put(SETTING_NUMBER_OF_REPLICAS, 0).put(LifecycleSettings.LIFECYCLE_NAME, "test").build(); Map phases = new HashMap<>(); - Map warmPhaseActions = Collections.singletonMap(ForceMergeAction.NAME, new ForceMergeAction(10000, false)); + Map warmPhaseActions = Collections.singletonMap(ForceMergeAction.NAME, new ForceMergeAction(10000)); phases.put("warm", new Phase("warm", TimeValue.timeValueSeconds(2), warmPhaseActions)); Map deletePhaseActions = Collections.singletonMap(DeleteAction.NAME, new DeleteAction()); diff --git a/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java b/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java index b4e7917a21b..593949c050e 100644 --- a/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java +++ b/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java @@ -110,7 +110,7 @@ public class TimeSeriesLifecycleActionsIT extends ESRestTestCase { }; assertThat(numSegments.get(), greaterThan(1)); - createNewSingletonPolicy("warm", new ForceMergeAction(1, false)); + createNewSingletonPolicy("warm", new ForceMergeAction(1)); updateIndexSettings(index, Settings.builder().put(LifecycleSettings.LIFECYCLE_NAME, policy)); assertBusy(() -> {