stop throwing an IllegalStateException for unrecognized steps by talevy · Pull Request #32212 · elastic/elasticsearch

talevy · 2018-07-19T20:19:26Z

Since the reason for a step not being found in a registry may be due to staleness of the
registry between it and the cluster state, we do not want to throw an IllegalStateException.

Staleness is something that will be self-healing after follow-up applications of the cluster state
updates, so this is a recoverable issue that should log a warning instead of throwing an exception

Closes #32181.

… runPolicy Since the reason for a step not being found in a registry may be due to staleness of the registry between it and the cluster state, we do not want to throw an IllegalStateException. Staleness is something that will be self-healing after follow-up applications of the cluster state updates, so this is a recoverable issue that should log a warning instead of throwing an exception Closes elastic#32181.

elasticmachine · 2018-07-19T20:19:28Z

Pinging @elastic/es-core-infra

talevy · 2018-07-19T20:20:55Z

...dex-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java

-                "current step for index [" + indexMetaData.getIndex().getName() + "] with policy [" + policy + "] is not recognized");
+            // This may happen in the case that there is invalid ilm-step index settings or the stepRegistry is out of
+            // sync with the current cluster state
+            logger.warn("current step [" + getCurrentStepKey(indexSettings) + "] for index [" + indexMetaData.getIndex().getName()


Although I love that we want to provide the step info here, I have mixed feelings about the assertions that exist inside of getCurrentStepKey. I think this should be changed to throwing IllegalStateException. what do you think @colings86?

I think the assertions are fine because ILM is in full control of the phase, action and step settings (they are INTERNAL settings so can't be touched by a user and will soon be moved to a custom index metadata object further locking down access to them. Therefore, I don't think its necessary to check that if one is set all three are set in production code but its useful to have the check in testing to make sure we don't do something silly so assertions feel right to me.

talevy · 2018-07-19T22:27:46Z

...dex-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java

@@ -64,8 +64,11 @@ public void runPolicy(String policy, IndexMetaData indexMetaData, ClusterState c
        }
        Step currentStep = getCurrentStep(stepRegistry, policy, indexSettings);


I looked closer at this. I missed something here.

This method calls PolicyStepsRegistry#getStep, which chooses to throw IllegalStateExceptions as well. Further investigation to see what the repercussions of changing that is necessary.

and tests need to be added to walk these paths as well. Current tests do not catch this

In other words, should https://github.com/talevy/elasticsearch/blob/80ab7739ca8877e2036edfac724f6014fefb7e52/x-pack/plugin/index-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/PolicyStepsRegistry.java#L101-L114 continue to throw an IllegalStateException?

I think we probably need to make PolicyStepsRegistry.getStep() return null if the step is missing (in much the same way that a map returns null for a missing key. Then we'll have to check for null:

here in IndexLifecycleRunner.runPolicy() and if its null log a warning and return

In IndexLifecycleRunner.moveClusterStateToStep() where I think we should do the same as above

In ExecuteStepsUpdateTask.execute() where we should log the warning if its null but return the cluster state up to changing the current step so we don't lose all the progress made on the previous cluster state steps

wdyt?

that is what I was thinking as well. Just wanted to double check with you before I take that route

colings86

@talevy I left a few comments

colings86 · 2018-07-27T08:48:54Z

...dex-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java

-                "current step for index [" + indexMetaData.getIndex().getName() + "] with policy [" + policy + "] is not recognized");
+            // This may happen in the case that there is invalid ilm-step index settings or the stepRegistry is out of
+            // sync with the current cluster state
+            logger.warn("current step [" + getCurrentStepKey(indexSettings) + "] for index [" + indexMetaData.getIndex().getName()


I think the assertions are fine because ILM is in full control of the phase, action and step settings (they are INTERNAL settings so can't be touched by a user and will soon be moved to a custom index metadata object further locking down access to them. Therefore, I don't think its necessary to check that if one is set all three are set in production code but its useful to have the check in testing to make sure we don't do something silly so assertions feel right to me.

colings86 · 2018-07-27T08:50:14Z

...dex-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java

-            throw new IllegalArgumentException(e.getMessage());
+        Step nextStep = stepRegistry.getStep(indexPolicySetting, nextStepKey);
+        if (nextStep == null) {
+            throw new IllegalArgumentException("step [" + nextStepKey + "] with policy [" + indexPolicySetting + "] does not exist");


Could you explain why this still needs to throw an exception rather than just returning an unmodified cluster state? Also I know we threw an IllegalArgumentException before but it feels wrong to me since the user won't have supplied anything invalid

I am cool with that.

I still feel like IllegalArgumentException should be thrown to users of TransportMoveToStepAction. So I will push up a basic exception for that, and it will know that nothing "right" happened since the clusterStates will be the same instance

… throw in action

colings86 · 2018-07-30T08:35:19Z

...dex-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java

        if (nextStep == null) {
-            throw new IllegalArgumentException("step [" + nextStepKey + "] with policy [" + indexPolicySetting + "] does not exist");
+            // stepRegistry may not be up-to-date with latest policies/steps in cluster-state
+            return currentState;


Sorry, I had not looked closely enough at what this method is used for. It seems that its only used for the "move to step" and retry APIs so actually its probably ok for this method to throw an exception directly. I had thought before that we use this method for moving between steps normally.

I think we should throw directly here as the alternative of returning the same cluster state and then using that as an indication that the step is not recognised is a bit trappy. To ensure this method is kept only for API calls and not for normal execution maybe we can add a JavaDoc comment?

OK. I will revert the latest commit and add comment

…ner, but throw in action" This reverts commit c009641.

colings86

LGTM

talevy · 2018-07-31T14:07:11Z

thanks!

Since the reason for a step not being found in a registry may be due to staleness of the registry between it and the cluster state, we do not want to throw an IllegalStateException. Staleness is something that will be self-healing after follow-up applications of the cluster state updates, so this is a recoverable issue that should log a warning instead of throwing an exception Closes #32181.

talevy added >bug :Data Management/ILM+SLM DO NOT USE. Use ":StorageEngine/ILM" or ":Distributed Coordination/SLM" instead. labels Jul 19, 2018

talevy commented Jul 19, 2018

View reviewed changes

talevy mentioned this pull request Jul 19, 2018

re-enable ILM integration tests and fix policyRegistry update bug #32108

Merged

talevy commented Jul 19, 2018

View reviewed changes

talevy added the WIP label Jul 19, 2018

talevy requested a review from colings86 July 20, 2018 15:39

talevy added 2 commits July 24, 2018 11:28

Merge branch 'index-lifecycle' into ilm-no-illegal-state

3235a1f

more nullity instead of throwing exceptions

5a96ade

talevy removed the WIP label Jul 24, 2018

talevy added 3 commits July 25, 2018 09:01

Merge branch 'index-lifecycle' into ilm-no-illegal-state

f19ba97

Merge branch 'index-lifecycle' into ilm-no-illegal-state

24ee939

fix rest test

81bf84c

talevy requested review from colings86 and dakrone and removed request for colings86 July 26, 2018 01:04

colings86 reviewed Jul 27, 2018

View reviewed changes

talevy added 2 commits July 27, 2018 14:34

Merge branch 'index-lifecycle' into ilm-no-illegal-state

c82a463

update to not throw exception when step not recognized in runner, but…

c009641

… throw in action

colings86 reviewed Jul 30, 2018

View reviewed changes

talevy added 3 commits July 30, 2018 10:20

Revert "update to not throw exception when step not recognized in run…

460fb34

…ner, but throw in action" This reverts commit c009641.

Merge branch 'index-lifecycle' into ilm-no-illegal-state

76f9245

add javadoc for moveClusterStateToStep

4bb6910

talevy requested a review from colings86 July 30, 2018 18:44

colings86 approved these changes Jul 31, 2018

View reviewed changes

talevy merged commit 6e9f338 into elastic:index-lifecycle Jul 31, 2018

talevy deleted the ilm-no-illegal-state branch July 31, 2018 14:06

talevy mentioned this pull request Jul 31, 2018

Test and Verify whether PolicyStepsRegistry is used safely in ILM #32181

Closed

		@@ -64,8 +64,11 @@ public void runPolicy(String policy, IndexMetaData indexMetaData, ClusterState c
		}
		Step currentStep = getCurrentStep(stepRegistry, policy, indexSettings);

Conversation

talevy commented Jul 19, 2018

Uh oh!

elasticmachine commented Jul 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Choose a reason for hiding this comment

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!

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!

colings86 left a comment

Choose a reason for hiding this comment

Uh oh!

talevy commented Jul 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

talevy Jul 19, 2018 •

edited

Loading