add version, policyname, and modifiedDate to ILM Explain API #33488

talevy · 2018-09-07T01:30:20Z

Adds the currently executing policy-name, its version, and its modified date to the explain api
for every executing/managed index

TODO:

find right place for integration test catching a phase mid-execution (so the info is rendered)
- decided that the right place is the IndexLifecycleIT Client tests. two birds with one stone.
update HLRC code to reflect server changes

elasticmachine · 2018-09-07T01:31:25Z

Pinging @elastic/es-core-infra

talevy · 2018-09-07T01:32:46Z

Hey @colings86, there is some more work to be done here, but I think there is enough to give a fair representation of the implementation direction. I'd appreciate the early feedback pending availability. It can wait until I finish off the TODOs too

colings86

@talevy I left a comment but I think this is good so far.

colings86 · 2018-09-07T08:13:58Z

...lugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/PhaseExecutionInfo.java

+    private static final ParseField PHASE_DEFINITION_FIELD = new ParseField("phase_definition");
+    private static final ParseField VERSION_FIELD = new ParseField("version");
+    private static final ParseField MODIFIED_DATE_FIELD = new ParseField("modified_date");
+    private static final ParseField MODIFIED_DATE_STRING_FIELD = new ParseField("modified_date_string");


Can we switch between the string and the millis representation fo the modified date using the human flag like the explain API already does? That way we can just have one modified_date field in the output? Also the parser will not need to worry about the string version in this case since the client it will never set the human flag

yes. I forgot this was an option until I noticed that is happening elsewhere in ILM objects. I will update accordingly

dakrone

Thanks for doing this @talevy, I left some comments

dakrone · 2018-09-14T17:31:16Z

...est-high-level/src/main/java/org/elasticsearch/client/indexlifecycle/PhaseExecutionInfo.java

+
+    @Override
+    public String toString() {
+        return Strings.toString(this, true, true);


Can you make this non-pretty, it's always weird when you log things and then end up being multi-line

true, log lines are unfortunate, but comparing test assertions are. since one is in production, logging concerns win 👍

dakrone · 2018-09-14T17:34:44Z

...est-high-level/src/main/java/org/elasticsearch/client/indexlifecycle/PhaseExecutionInfo.java

+        builder.field(POLICY_NAME_FIELD.getPreferredName(), policyName);
+        builder.field(PHASE_DEFINITION_FIELD.getPreferredName(), phase);
+        builder.field(VERSION_FIELD.getPreferredName(), version);
+        if (builder.humanReadable()) {


For human readable-ness, an additional field should be added, we shouldn't replace the field with a human readable version.

You should be able to do

builder.timeField("modified_millis", "modified", modifiedDate);

(replacing the field names with the fields we want to use)

and then you don't have to check the human readable flag yourself

...h-level/src/test/java/org/elasticsearch/client/indexlifecycle/IndexExplainResponseTests.java

dakrone · 2018-09-14T17:39:44Z

...lugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/PhaseExecutionInfo.java

+
+    @Override
+    public String toString() {
+        return Strings.toString(this, true, true);


Same here about multi-line toString methods

dakrone · 2018-09-14T17:40:04Z

...lugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/PhaseExecutionInfo.java

+        builder.field(POLICY_NAME_FIELD.getPreferredName(), policyName);
+        builder.field(PHASE_DEFINITION_FIELD.getPreferredName(), phase);
+        builder.field(VERSION_FIELD.getPreferredName(), version);
+        if (builder.humanReadable()) {


Here again I think we should use builder.timeField to handle this

dakrone · 2018-09-14T17:45:12Z

...main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportExplainLifecycleAction.java

+                        DeprecationHandler.THROW_UNSUPPORTED_OPERATION, phaseDef)) {
+                    phaseExecutionInfo = PhaseExecutionInfo.parse(parser, currentPhase);
+                } catch (IOException e) {
+                    logger.error("failed to parse [" + LifecycleSettings.LIFECYCLE_PHASE_DEFINITION + "] for index [" + index + "]", e);


I don't think we should swallow this error here, I think the request should error out also

x-pack/plugin/src/test/resources/rest-api-spec/test/ilm/40_explain_lifecycle.yml

dakrone

LGTM, left a super minor comment

...lugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/PhaseExecutionInfo.java

talevy · 2018-09-17T23:28:52Z

thanks for the review @dakrone!

adds a section for phase execution to the Explain API. This contains - phase definition - policy name - policy version - modified date

add version, policyname, and modifiedDate to ILM Explain API

bbda2d7

talevy added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label Sep 7, 2018

talevy added the WIP label Sep 7, 2018

talevy requested a review from colings86 September 7, 2018 01:31

elasticmachine mentioned this pull request Sep 7, 2018

[meta] Index Lifecycle Management Plan #29823

Closed

colings86 reviewed Sep 7, 2018

View reviewed changes

talevy added 7 commits September 7, 2018 12:22

add extra checks for emptiness of phase_execution

89076b7

Merge branch 'index-lifecycle' into ilm-explain-more

4bb3abd

make client happy

0b04b98

Merge branch 'index-lifecycle' into ilm-explain-more

44c4dd8

add integration test for phase_execution_info

8f4676a

Merge branch 'index-lifecycle' into ilm-explain-more

0d5ee66

final adjustments to make tests pass and fix merge changes

4efb17e

talevy requested review from dakrone and gwbrown September 13, 2018 19:05

talevy removed the WIP label Sep 13, 2018

talevy added 3 commits September 13, 2018 12:07

fix humanReadable toXContent in client

17b4b4b

fix tests

799703b

Merge branch 'master' into index-lifecycle

0f978b4

dakrone requested changes Sep 14, 2018

View reviewed changes

talevy added 4 commits September 14, 2018 11:23

Merge branch 'index-lifecycle' into ilm-explain-more

8299460

clean up parsing

0dd583b

Merge branch 'index-lifecycle' into ilm-explain-more

8255014

fix style

6ba3516

talevy requested a review from dakrone September 17, 2018 21:57

dakrone approved these changes Sep 17, 2018

View reviewed changes

...lugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/PhaseExecutionInfo.java Show resolved Hide resolved

...lugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/PhaseExecutionInfo.java Show resolved Hide resolved

add javadoc

d6cf11f

talevy merged commit 94a66c5 into elastic:index-lifecycle Sep 18, 2018

talevy deleted the ilm-explain-more branch September 18, 2018 00:00

talevy added a commit that referenced this pull request Sep 18, 2018

add phase execution info to ILM Explain API (#33488)

7a19e26

adds a section for phase execution to the Explain API. This contains - phase definition - policy name - policy version - modified date

add version, policyname, and modifiedDate to ILM Explain API #33488

add version, policyname, and modifiedDate to ILM Explain API #33488

Uh oh!

Conversation

talevy commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 7, 2018

Uh oh!

talevy commented Sep 7, 2018

Uh oh!

colings86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

talevy commented Sep 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

talevy commented Sep 7, 2018 •

edited

Loading