Adding best_compression #49974

SivagurunathanV · 2019-12-08T22:54:55Z

Best_Compression in ILM policy

elasticmachine · 2019-12-08T23:28:01Z

Pinging @elastic/es-core-features (:Core/Features/ILM+SLM)

gwbrown

Thanks for the contribution! I haven't had time to look through it completely (I or @dakrone will do that soon), but it looks like a good take on the problem. However, I did notice an issue with the license headers, as described in the comments.

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CloseIndexStep.java

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/OpenIndexStep.java

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForIndexGreenStep.java

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/CloseIndexStepTest.java

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/OpenIndexStepTest.java

...ck/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForIndexGreenStepTest.java

SivagurunathanV · 2019-12-10T02:53:42Z

@gwbrown I made the changes as requested. 👍

dakrone

Thanks for working on this @SivagurunathanV! I left some comments

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CloseIndexStep.java

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

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CloseIndexStep.java

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/OpenIndexStep.java

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForIndexGreenStep.java

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

dakrone · 2019-12-10T23:16:41Z

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

+        if(randomBoolean()) {
+            maxNumSegments = maxNumSegments + randomIntBetween(1, 10);
+        }
+        else {


Extra newline before this :)

jasontedor

I left comment.

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

SivagurunathanV · 2019-12-17T04:43:28Z

@dakrone @jasontedor 👋 Made changes as per the review comments. Please have a look.

jasontedor

At a high-level, this is looking good to me, thanks for responding to the feedback. I left one comment. Also, missing for me is a documentation update. Can you include that too (e.g., policy-definitions.asciidoc). I didn't do a detailed review of the code, I'll leave that to @dakrone.

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

SivagurunathanV · 2019-12-18T06:03:57Z

@jasontedor Applied the index_codec change 👍

dakrone

Thanks for addressing the feedback @SivagurunathanV, I left some more comments.

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

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

dakrone · 2019-12-18T23:41:41Z

...plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java

@@ -492,7 +493,7 @@ private void assertInvalidAction(String phaseName, String currentAction, String.
            case DeleteAction.NAME:
                return new DeleteAction();
            case ForceMergeAction.NAME:
-                return new ForceMergeAction(1);
+                return new ForceMergeAction(1, Codec.getDefault());


These should pass null (or have a second constructor so there's no need to specify the codec)

dakrone · 2019-12-18T23:42:44Z

...lm/qa/multi-node/src/test/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java

@@ -411,7 +412,7 @@ public void testForceMergeAction() throws Exception {
        };
        assertThat(numSegments.get(), greaterThan(1));

-        createNewSingletonPolicy("warm", new ForceMergeAction(1));
+        createNewSingletonPolicy("warm", new ForceMergeAction(1, Codec.getDefault()));


Can you add an integration test for the best_compression codec here? (in a separate test method)

Thinking of adding codec as parameterized and use this test for best_compression as well. What you think?

SivagurunathanV · 2019-12-20T06:49:20Z

Made changes as per review comments. Let me know. Thanks 👍

dakrone

@SivagurunathanV thanks for the updates (sorry about the lag in review due to holidays). I left some more feedback, but it's fairly minor. This is getting close.

dakrone · 2020-01-09T22:55:52Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CloseIndexStep.java

+            CloseIndexRequest request = new CloseIndexRequest(indexMetaData.getIndex().getName());
+            getClient().admin().indices()
+                .close(request, ActionListener.wrap(closeIndexResponse -> {
+                    assert closeIndexResponse.isAcknowledged() : "close index response is not acknowledged";


I think rather than an assert, this should throw an exception and go into the ERROR step

Added Error Step.

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CloseIndexStep.java

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

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/OpenIndexStep.java

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForIndexColorStep.java

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

...plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java

...lm/qa/multi-node/src/test/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForIndexColorStep.java

dakrone · 2020-01-15T22:29:14Z

@SivagurunathanV I saw you pushed some changes to this branch, should I review it again, or do you want me to wait until you request another review?

SivagurunathanV · 2020-01-16T00:43:58Z

@dakrone Yeah sure, you can review this and let me know if you have any comments.

dakrone

We're really close! I left two more comments, thanks for going back and forth on this @SivagurunathanV!

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CloseIndexStep.java

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/OpenIndexStep.java

dakrone · 2020-01-17T22:04:52Z

@elasticmachine ok to test

SivagurunathanV · 2020-01-19T18:43:08Z

Added exception handling, struck on the failing tests now.

PR elastic#50333 has been merged.

DFRSimilarity -> DFR similarity

SivagurunathanV · 2020-01-31T08:18:14Z

@dakrone Fixed the broken test. Please review it and let me know.

dakrone

This looks good to me now! I left 2 super minor comments (they should be really easy to address), if you want to change those then I will merge this (I'll probably open a separate PR for adding it to the docs unless you want to do that).

Thanks for iterating on this @SivagurunathanV!

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

(stale)

SivagurunathanV · 2020-02-01T02:17:19Z

@dakrone Thanks for the feedback. Please review it and let me know.

This commit adds a `codec` parameter to the ILM `forcemerge` action. When setting the codec to `best_compression` ILM will close the index, then update the codec setting, re-open the index, and finally perform a force merge.

dakrone · 2020-02-03T15:42:27Z

@SivagurunathanV this has been merged, thank you for all your iterations! I'm working on the backport and then I'll update the docs so folks can learn about this.

SivagurunathanV · 2020-02-03T17:03:49Z

Sure, Please let me know if you need any help on this or assign it to me.

Thanks

* Adding best_compression (#49974) This commit adds a `codec` parameter to the ILM `forcemerge` action. When setting the codec to `best_compression` ILM will close the index, then update the codec setting, re-open the index, and finally perform a force merge. * Fix ForceMergeAction toSteps construction (#51825) There was a duplicate force merge step and the test continued to fail. This commit clarifies the `toStep` method and changes the `assertBestCompression` method for better readability. Resolves #51822 * Update version constants Co-authored-by: Sivagurunathan Velayutham <sivadeva.93@gmail.com>

This adds the option to the parameter list and a warning about the index being unavailable during the close and open operations. Relates to elastic#49974

…tic#51819)" This reverts commit 0be61a3.

This adds the option to the parameter list and a warning about the index being unavailable during the close and open operations. Relates to #49974

SivagurunathanV marked this pull request as ready for review December 8, 2019 22:56

SivagurunathanV mentioned this pull request Dec 8, 2019

Add best_compression back to ILM policy #41274

Closed

tvernum added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label Dec 8, 2019

SivagurunathanV force-pushed the master branch from c079674 to 5534513 Compare December 9, 2019 05:35

dakrone self-requested a review December 9, 2019 16:43

gwbrown previously requested changes Dec 10, 2019

View reviewed changes

gwbrown self-requested a review December 10, 2019 04:23

dakrone requested changes Dec 10, 2019

View reviewed changes

jasontedor reviewed Dec 11, 2019

View reviewed changes

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/ForceMergeAction.java Outdated Show resolved Hide resolved

jasontedor reviewed Dec 17, 2019

View reviewed changes

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/ForceMergeAction.java Outdated Show resolved Hide resolved

dakrone self-requested a review December 17, 2019 15:04

dakrone requested changes Dec 18, 2019

View reviewed changes

dakrone self-requested a review January 2, 2020 23:13

dakrone requested changes Jan 9, 2020

View reviewed changes

dakrone added >enhancement v8.0.0 labels Jan 9, 2020

dakrone reviewed Jan 9, 2020

View reviewed changes

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForIndexColorStep.java Show resolved Hide resolved

dakrone requested changes Jan 17, 2020

View reviewed changes

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CloseIndexStep.java Outdated Show resolved Hide resolved

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/OpenIndexStep.java Outdated Show resolved Hide resolved

jpountz and others added 3 commits January 20, 2020 19:34

Upgrade to Lucene 8.4.0. (elastic#50518)

3f3f4ee

Reenable bw tests after merging elastic#50333. (elastic#50746)

1d882c9

PR elastic#50333 has been merged.

[Docs] Update similarity.asciidoc (elastic#50719)

bca8790

DFRSimilarity -> DFR similarity

SivagurunathanV added 2 commits January 30, 2020 23:12

Removing compression test

3380459

Remove unused import

766ed8e

dakrone approved these changes Jan 31, 2020

View reviewed changes

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/ForceMergeAction.java Outdated Show resolved Hide resolved

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/ForceMergeAction.java Outdated Show resolved Hide resolved

dakrone added the v7.7.0 label Jan 31, 2020

SivagurunathanV added 3 commits January 31, 2020 16:27

review comments

c3ec0b2

Formatting the code :face_palm

f32b604

Merge remote-tracking branch 'refs/remotes/upstream/master'

aa395cd

dakrone merged commit 763480e into elastic:master Feb 3, 2020

dakrone mentioned this pull request Feb 3, 2020

[7.x] Adding best_compression (#49974) (763480ee) #51819

Merged

dakrone added the backport pending label Feb 3, 2020

andreidan mentioned this pull request Feb 3, 2020

ForceMergeActionTests.testToSteps failure #51822

Closed

dakrone mentioned this pull request Feb 4, 2020

Add documentation about ILM forcemerge with best_compression #51893

Merged

dakrone added a commit to dakrone/elasticsearch that referenced this pull request Feb 4, 2020

Revert "[7.x] Adding best_compression (elastic#49974) (763480e) (elas…

fe43586

…tic#51819)" This reverts commit 0be61a3.

dakrone added a commit that referenced this pull request Feb 5, 2020

Add documentation about ILM forcemerge with best_compression (#51893)

b475b28

This adds the option to the parameter list and a warning about the index being unavailable during the close and open operations. Relates to #49974

dakrone added a commit that referenced this pull request Feb 5, 2020

Add documentation about ILM forcemerge with best_compression (#51893)

f8c8a10

This adds the option to the parameter list and a warning about the index being unavailable during the close and open operations. Relates to #49974

dakrone removed the backport pending label Feb 5, 2020

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 2) elastic/elasticsearch-net#4533

Closed

freakingid mentioned this pull request Apr 29, 2020

Remove codec from ilm action forcemerge #55978

Closed

DaveCTurner mentioned this pull request May 21, 2020

Confusion over codec parameter to ILM force-merge action #57034

Closed

sebelga mentioned this pull request Sep 8, 2020

[ILM] Add support for "codec" in the force merge elastic/kibana#76907

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding best_compression #49974

Adding best_compression #49974

SivagurunathanV commented Dec 8, 2019 •

edited

Loading

elasticmachine commented Dec 8, 2019

gwbrown left a comment

SivagurunathanV commented Dec 10, 2019

dakrone left a comment

dakrone Dec 10, 2019

jasontedor left a comment

SivagurunathanV commented Dec 17, 2019

jasontedor left a comment

SivagurunathanV commented Dec 18, 2019

dakrone left a comment

dakrone Dec 18, 2019

dakrone Dec 18, 2019

SivagurunathanV Dec 20, 2019

SivagurunathanV commented Dec 20, 2019

dakrone left a comment

dakrone Jan 9, 2020

SivagurunathanV Jan 10, 2020

dakrone commented Jan 15, 2020

SivagurunathanV commented Jan 16, 2020

dakrone left a comment

dakrone commented Jan 17, 2020

SivagurunathanV commented Jan 19, 2020

SivagurunathanV commented Jan 31, 2020

dakrone left a comment

SivagurunathanV commented Feb 1, 2020

dakrone commented Feb 3, 2020

SivagurunathanV commented Feb 3, 2020

Adding best_compression #49974

Adding best_compression #49974

Conversation

SivagurunathanV commented Dec 8, 2019 • edited Loading

elasticmachine commented Dec 8, 2019

gwbrown left a comment

Choose a reason for hiding this comment

SivagurunathanV commented Dec 10, 2019

dakrone left a comment

Choose a reason for hiding this comment

dakrone Dec 10, 2019

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

SivagurunathanV commented Dec 17, 2019

jasontedor left a comment

Choose a reason for hiding this comment

SivagurunathanV commented Dec 18, 2019

dakrone left a comment

Choose a reason for hiding this comment

dakrone Dec 18, 2019

Choose a reason for hiding this comment

dakrone Dec 18, 2019

Choose a reason for hiding this comment

SivagurunathanV Dec 20, 2019

Choose a reason for hiding this comment

SivagurunathanV commented Dec 20, 2019

dakrone left a comment

Choose a reason for hiding this comment

dakrone Jan 9, 2020

Choose a reason for hiding this comment

SivagurunathanV Jan 10, 2020

Choose a reason for hiding this comment

dakrone commented Jan 15, 2020

SivagurunathanV commented Jan 16, 2020

dakrone left a comment

Choose a reason for hiding this comment

dakrone commented Jan 17, 2020

SivagurunathanV commented Jan 19, 2020

SivagurunathanV commented Jan 31, 2020

dakrone left a comment

Choose a reason for hiding this comment

SivagurunathanV commented Feb 1, 2020

dakrone commented Feb 3, 2020

SivagurunathanV commented Feb 3, 2020

SivagurunathanV commented Dec 8, 2019 •

edited

Loading