Skip to content

Commit 3daefe6

Browse files
dakronedakrone
authored
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
1 parent a75c6a2 commit 3daefe6

File tree

7 files changed

+25
-93
lines changed

7 files changed

+25
-93
lines changed

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/ForceMergeAction.java

Lines changed: 5 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,9 @@
1010
import org.elasticsearch.common.Strings;
1111
import org.elasticsearch.common.io.stream.StreamInput;
1212
import org.elasticsearch.common.io.stream.StreamOutput;
13-
import org.elasticsearch.common.settings.Settings;
1413
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
1514
import org.elasticsearch.common.xcontent.XContentBuilder;
1615
import org.elasticsearch.common.xcontent.XContentParser;
17-
import org.elasticsearch.index.codec.CodecService;
18-
import org.elasticsearch.index.engine.EngineConfig;
1916
import org.elasticsearch.xpack.core.indexlifecycle.Step.StepKey;
2017

2118
import java.io.IOException;
@@ -29,53 +26,42 @@
2926
public class ForceMergeAction implements LifecycleAction {
3027
public static final String NAME = "forcemerge";
3128
public static final ParseField MAX_NUM_SEGMENTS_FIELD = new ParseField("max_num_segments");
32-
public static final ParseField BEST_COMPRESSION_FIELD = new ParseField("best_compression");
3329

3430
private static final ConstructingObjectParser<ForceMergeAction, Void> PARSER = new ConstructingObjectParser<>(NAME,
3531
false, a -> {
3632
int maxNumSegments = (int) a[0];
37-
boolean bestCompression = a[1] == null ? false : (boolean) a[1];
38-
return new ForceMergeAction(maxNumSegments, bestCompression);
33+
return new ForceMergeAction(maxNumSegments);
3934
});
4035

4136
static {
4237
PARSER.declareInt(ConstructingObjectParser.constructorArg(), MAX_NUM_SEGMENTS_FIELD);
43-
PARSER.declareBoolean(ConstructingObjectParser.optionalConstructorArg(), BEST_COMPRESSION_FIELD);
4438
}
4539

4640
private final int maxNumSegments;
47-
private final boolean bestCompression;
4841

4942
public static ForceMergeAction parse(XContentParser parser) {
5043
return PARSER.apply(parser, null);
5144
}
5245

53-
public ForceMergeAction(int maxNumSegments, boolean bestCompression) {
46+
public ForceMergeAction(int maxNumSegments) {
5447
if (maxNumSegments <= 0) {
5548
throw new IllegalArgumentException("[" + MAX_NUM_SEGMENTS_FIELD.getPreferredName()
5649
+ "] must be a positive integer");
5750
}
5851
this.maxNumSegments = maxNumSegments;
59-
this.bestCompression = bestCompression;
6052
}
6153

6254
public ForceMergeAction(StreamInput in) throws IOException {
6355
this.maxNumSegments = in.readVInt();
64-
this.bestCompression = in.readBoolean();
6556
}
6657

6758
public int getMaxNumSegments() {
6859
return maxNumSegments;
6960
}
7061

71-
public boolean isBestCompression() {
72-
return bestCompression;
73-
}
74-
7562
@Override
7663
public void writeTo(StreamOutput out) throws IOException {
7764
out.writeVInt(maxNumSegments);
78-
out.writeBoolean(bestCompression);
7965
}
8066

8167
@Override
@@ -92,7 +78,6 @@ public boolean isSafeAction() {
9278
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
9379
builder.startObject();
9480
builder.field(MAX_NUM_SEGMENTS_FIELD.getPreferredName(), maxNumSegments);
95-
builder.field(BEST_COMPRESSION_FIELD.getPreferredName(), bestCompression);
9681
builder.endObject();
9782
return builder;
9883
}
@@ -103,20 +88,13 @@ public List<Step> toSteps(Client client, String phase, Step.StepKey nextStepKey)
10388
StepKey forceMergeKey = new StepKey(phase, NAME, ForceMergeStep.NAME);
10489
StepKey countKey = new StepKey(phase, NAME, SegmentCountStep.NAME);
10590
ForceMergeStep forceMergeStep = new ForceMergeStep(forceMergeKey, countKey, client, maxNumSegments);
106-
SegmentCountStep segmentCountStep = new SegmentCountStep(countKey, nextStepKey, client, maxNumSegments, bestCompression);
107-
if (bestCompression) {
108-
Settings compressionSettings = Settings.builder()
109-
.put(EngineConfig.INDEX_CODEC_SETTING.getKey(), CodecService.BEST_COMPRESSION_CODEC).build();
110-
UpdateSettingsStep updateBestCompression = new UpdateSettingsStep(updateCompressionKey,
111-
forceMergeKey, client, compressionSettings);
112-
return Arrays.asList(updateBestCompression, forceMergeStep, segmentCountStep);
113-
}
91+
SegmentCountStep segmentCountStep = new SegmentCountStep(countKey, nextStepKey, client, maxNumSegments);
11492
return Arrays.asList(forceMergeStep, segmentCountStep);
11593
}
11694

11795
@Override
11896
public int hashCode() {
119-
return Objects.hash(maxNumSegments, bestCompression);
97+
return Objects.hash(maxNumSegments);
12098
}
12199

122100
@Override
@@ -128,8 +106,7 @@ public boolean equals(Object obj) {
128106
return false;
129107
}
130108
ForceMergeAction other = (ForceMergeAction) obj;
131-
return Objects.equals(maxNumSegments, other.maxNumSegments)
132-
&& Objects.equals(bestCompression, other.bestCompression);
109+
return Objects.equals(maxNumSegments, other.maxNumSegments);
133110
}
134111

135112
@Override

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/SegmentCountStep.java

Lines changed: 4 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -27,48 +27,28 @@ public class SegmentCountStep extends AsyncWaitStep {
2727
public static final String NAME = "segment-count";
2828

2929
private final int maxNumSegments;
30-
private final boolean bestCompression;
3130

32-
public SegmentCountStep(StepKey key, StepKey nextStepKey, Client client, int maxNumSegments, boolean bestCompression) {
31+
public SegmentCountStep(StepKey key, StepKey nextStepKey, Client client, int maxNumSegments) {
3332
super(key, nextStepKey, client);
3433
this.maxNumSegments = maxNumSegments;
35-
this.bestCompression = bestCompression;
3634
}
3735

3836
public int getMaxNumSegments() {
3937
return maxNumSegments;
4038
}
4139

42-
public boolean isBestCompression() {
43-
return bestCompression;
44-
}
45-
4640
@Override
4741
public void evaluateCondition(Index index, Listener listener) {
4842
getClient().admin().indices().segments(new IndicesSegmentsRequest(index.getName()), ActionListener.wrap(response -> {
4943
long numberShardsLeftToMerge = StreamSupport.stream(response.getIndices().get(index.getName()).spliterator(), false)
50-
.filter(iss -> Arrays.stream(iss.getShards()).anyMatch(p -> {
51-
boolean hasRightAmountOfSegments = p.getSegments().size() <= maxNumSegments;
52-
if (bestCompression) {
53-
// // TODO(talevy): discuss
54-
// boolean allUsingCorrectCompression = p.getSegments().stream().anyMatch(s ->
55-
// Lucene50StoredFieldsFormat.Mode.BEST_COMPRESSION.equals(
56-
// Lucene50StoredFieldsFormat.Mode.BEST_COMPRESSION.toString().equals(
57-
// s.getAttributes().get(Lucene50StoredFieldsFormat.MODE_KEY)))
58-
// );
59-
boolean allUsingCorrectCompression = true;
60-
return (hasRightAmountOfSegments && allUsingCorrectCompression) == false;
61-
} else {
62-
return hasRightAmountOfSegments == false;
63-
}
64-
})).count();
44+
.filter(iss -> Arrays.stream(iss.getShards()).anyMatch(p -> p.getSegments().size() > maxNumSegments)).count();
6545
listener.onResponse(numberShardsLeftToMerge == 0, new Info(numberShardsLeftToMerge));
6646
}, listener::onFailure));
6747
}
6848

6949
@Override
7050
public int hashCode() {
71-
return Objects.hash(super.hashCode(), maxNumSegments, bestCompression);
51+
return Objects.hash(super.hashCode(), maxNumSegments);
7252
}
7353

7454
@Override
@@ -81,8 +61,7 @@ public boolean equals(Object obj) {
8161
}
8262
SegmentCountStep other = (SegmentCountStep) obj;
8363
return super.equals(obj)
84-
&& Objects.equals(maxNumSegments, other.maxNumSegments)
85-
&& Objects.equals(bestCompression, other.bestCompression);
64+
&& Objects.equals(maxNumSegments, other.maxNumSegments);
8665
}
8766

8867
public static class Info implements ToXContentObject {

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/ForceMergeActionTests.java

Lines changed: 5 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77

88
import org.elasticsearch.common.bytes.BytesReference;
99
import org.elasticsearch.common.io.stream.Writeable.Reader;
10-
import org.elasticsearch.common.settings.Settings;
1110
import org.elasticsearch.common.xcontent.DeprecationHandler;
1211
import org.elasticsearch.common.xcontent.XContentHelper;
1312
import org.elasticsearch.common.xcontent.XContentParser;
@@ -29,19 +28,14 @@ protected ForceMergeAction doParseInstance(XContentParser parser) {
2928

3029
@Override
3130
protected ForceMergeAction createTestInstance() {
32-
return new ForceMergeAction(randomIntBetween(1, 100), randomBoolean());
31+
return new ForceMergeAction(randomIntBetween(1, 100));
3332
}
3433

3534
@Override
3635
protected ForceMergeAction mutateInstance(ForceMergeAction instance) {
3736
int maxNumSegments = instance.getMaxNumSegments();
38-
boolean bestCompression = instance.isBestCompression();
39-
if (randomBoolean()) {
40-
maxNumSegments = maxNumSegments + randomIntBetween(1, 10);
41-
} else {
42-
bestCompression = !bestCompression;
43-
}
44-
return new ForceMergeAction(maxNumSegments, bestCompression);
37+
maxNumSegments = maxNumSegments + randomIntBetween(1, 10);
38+
return new ForceMergeAction(maxNumSegments);
4539
}
4640

4741
@Override
@@ -58,7 +52,7 @@ public void testMissingMaxNumSegments() throws IOException {
5852
}
5953

6054
public void testInvalidNegativeSegmentNumber() {
61-
Exception r = expectThrows(IllegalArgumentException.class, () -> new ForceMergeAction(randomIntBetween(-10, 0), false));
55+
Exception r = expectThrows(IllegalArgumentException.class, () -> new ForceMergeAction(randomIntBetween(-10, 0)));
6256
assertThat(r.getMessage(), equalTo("[max_num_segments] must be a positive integer"));
6357
}
6458

@@ -69,16 +63,7 @@ public void testToSteps() {
6963
List<Step> steps = instance.toSteps(null, phase, nextStepKey);
7064
assertNotNull(steps);
7165
int nextFirstIndex = 0;
72-
if (instance.isBestCompression()) {
73-
Settings expectedSettings = Settings.builder().put("index.codec", "best_compression").build();
74-
assertEquals(3, steps.size());
75-
UpdateSettingsStep firstStep = (UpdateSettingsStep) steps.get(0);
76-
assertThat(firstStep.getKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, "best_compression")));
77-
assertThat(firstStep.getSettings(), equalTo(expectedSettings));
78-
nextFirstIndex = 1;
79-
} else {
80-
assertEquals(2, steps.size());
81-
}
66+
assertEquals(2, steps.size());
8267
ForceMergeStep firstStep = (ForceMergeStep) steps.get(nextFirstIndex);
8368
SegmentCountStep secondStep = (SegmentCountStep) steps.get(nextFirstIndex + 1);
8469
assertThat(firstStep.getKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, ForceMergeStep.NAME)));

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/SegmentCountStepTests.java

Lines changed: 7 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -37,19 +37,17 @@ public SegmentCountStep createRandomInstance() {
3737
Step.StepKey stepKey = randomStepKey();
3838
StepKey nextStepKey = randomStepKey();
3939
int maxNumSegments = randomIntBetween(1, 10);
40-
boolean bestCompression = randomBoolean();
4140

42-
return new SegmentCountStep(stepKey, nextStepKey, null, maxNumSegments, bestCompression);
41+
return new SegmentCountStep(stepKey, nextStepKey, null, maxNumSegments);
4342
}
4443

4544
@Override
4645
public SegmentCountStep mutateInstance(SegmentCountStep instance) {
4746
StepKey key = instance.getKey();
4847
StepKey nextKey = instance.getNextStepKey();
4948
int maxNumSegments = instance.getMaxNumSegments();
50-
boolean bestCompression = instance.isBestCompression();
5149

52-
switch (between(0, 3)) {
50+
switch (between(0, 2)) {
5351
case 0:
5452
key = new StepKey(key.getPhase(), key.getAction(), key.getName() + randomAlphaOfLength(5));
5553
break;
@@ -59,20 +57,16 @@ public SegmentCountStep mutateInstance(SegmentCountStep instance) {
5957
case 2:
6058
maxNumSegments += 1;
6159
break;
62-
case 3:
63-
bestCompression = !bestCompression;
64-
break;
6560
default:
6661
throw new AssertionError("Illegal randomisation branch");
6762
}
6863

69-
return new SegmentCountStep(key, nextKey, null, maxNumSegments, bestCompression);
64+
return new SegmentCountStep(key, nextKey, null, maxNumSegments);
7065
}
7166

7267
@Override
7368
public SegmentCountStep copyInstance(SegmentCountStep instance) {
74-
return new SegmentCountStep(instance.getKey(), instance.getNextStepKey(),
75-
null, instance.getMaxNumSegments(), instance.isBestCompression());
69+
return new SegmentCountStep(instance.getKey(), instance.getNextStepKey(), null, instance.getMaxNumSegments());
7670
}
7771

7872
public void testIsConditionMet() {
@@ -103,7 +97,6 @@ public void testIsConditionMet() {
10397

10498
Step.StepKey stepKey = randomStepKey();
10599
StepKey nextStepKey = randomStepKey();
106-
boolean bestCompression = randomBoolean();
107100

108101
Mockito.doAnswer(invocationOnMock -> {
109102
@SuppressWarnings("unchecked")
@@ -115,7 +108,7 @@ public void testIsConditionMet() {
115108
SetOnce<Boolean> conditionMetResult = new SetOnce<>();
116109
SetOnce<ToXContentObject> conditionInfo = new SetOnce<>();
117110

118-
SegmentCountStep step = new SegmentCountStep(stepKey, nextStepKey, client, maxNumSegments, bestCompression);
111+
SegmentCountStep step = new SegmentCountStep(stepKey, nextStepKey, client, maxNumSegments);
119112
step.evaluateCondition(index, new AsyncWaitStep.Listener() {
120113
@Override
121114
public void onResponse(boolean conditionMet, ToXContentObject info) {
@@ -161,7 +154,6 @@ public void testIsConditionFails() {
161154

162155
Step.StepKey stepKey = randomStepKey();
163156
StepKey nextStepKey = randomStepKey();
164-
boolean bestCompression = randomBoolean();
165157

166158
Mockito.doAnswer(invocationOnMock -> {
167159
@SuppressWarnings("unchecked")
@@ -173,7 +165,7 @@ public void testIsConditionFails() {
173165
SetOnce<Boolean> conditionMetResult = new SetOnce<>();
174166
SetOnce<ToXContentObject> conditionInfo = new SetOnce<>();
175167

176-
SegmentCountStep step = new SegmentCountStep(stepKey, nextStepKey, client, maxNumSegments, bestCompression);
168+
SegmentCountStep step = new SegmentCountStep(stepKey, nextStepKey, client, maxNumSegments);
177169
step.evaluateCondition(index, new AsyncWaitStep.Listener() {
178170
@Override
179171
public void onResponse(boolean conditionMet, ToXContentObject info) {
@@ -203,7 +195,6 @@ public void testThrowsException() {
203195
Step.StepKey stepKey = randomStepKey();
204196
StepKey nextStepKey = randomStepKey();
205197
int maxNumSegments = randomIntBetween(3, 10);
206-
boolean bestCompression = randomBoolean();
207198

208199
Mockito.doAnswer(invocationOnMock -> {
209200
@SuppressWarnings("unchecked")
@@ -214,7 +205,7 @@ public void testThrowsException() {
214205

215206
SetOnce<Boolean> exceptionThrown = new SetOnce<>();
216207

217-
SegmentCountStep step = new SegmentCountStep(stepKey, nextStepKey, client, maxNumSegments, bestCompression);
208+
SegmentCountStep step = new SegmentCountStep(stepKey, nextStepKey, client, maxNumSegments);
218209
step.evaluateCondition(index, new AsyncWaitStep.Listener() {
219210
@Override
220211
public void onResponse(boolean conditionMet, ToXContentObject info) {

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/TimeseriesLifecycleTypeTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,10 @@
2828
import static org.hamcrest.Matchers.equalTo;
2929

3030
public class TimeseriesLifecycleTypeTests extends ESTestCase {
31-
31+
3232
private static final AllocateAction TEST_ALLOCATE_ACTION = new AllocateAction(Collections.singletonMap("node", "node1"),null, null);
3333
private static final DeleteAction TEST_DELETE_ACTION = new DeleteAction();
34-
private static final ForceMergeAction TEST_FORCE_MERGE_ACTION = new ForceMergeAction(1, true);
34+
private static final ForceMergeAction TEST_FORCE_MERGE_ACTION = new ForceMergeAction(1);
3535
private static final ReplicasAction TEST_REPLICAS_ACTION = new ReplicasAction(1);
3636
private static final RolloverAction TEST_ROLLOVER_ACTION = new RolloverAction(new ByteSizeValue(1), null, null);
3737
private static final ShrinkAction TEST_SHRINK_ACTION = new ShrinkAction(1);

‎x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleInitialisationIT.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ public void init() {
9797
.put(SETTING_NUMBER_OF_REPLICAS, 0).put(LifecycleSettings.LIFECYCLE_NAME, "test").build();
9898
Map<String, Phase> phases = new HashMap<>();
9999

100-
Map<String, LifecycleAction> warmPhaseActions = Collections.singletonMap(ForceMergeAction.NAME, new ForceMergeAction(10000, false));
100+
Map<String, LifecycleAction> warmPhaseActions = Collections.singletonMap(ForceMergeAction.NAME, new ForceMergeAction(10000));
101101
phases.put("warm", new Phase("warm", TimeValue.timeValueSeconds(2), warmPhaseActions));
102102

103103
Map<String, LifecycleAction> deletePhaseActions = Collections.singletonMap(DeleteAction.NAME, new DeleteAction());

‎x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ public void testForceMergeAction() throws Exception {
110110
};
111111
assertThat(numSegments.get(), greaterThan(1));
112112

113-
createNewSingletonPolicy("warm", new ForceMergeAction(1, false));
113+
createNewSingletonPolicy("warm", new ForceMergeAction(1));
114114
updateIndexSettings(index, Settings.builder().put(LifecycleSettings.LIFECYCLE_NAME, policy));
115115

116116
assertBusy(() -> {

0 commit comments

Comments
 (0)