Skip to content

Do not inject MigrateAction if it is not supported #89457

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Sep 15, 2022
Merged

Do not inject MigrateAction if it is not supported #89457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7b41121
do not inject MigrateAction if it is not supported
mushao999 Aug 18, 2022
d8fc9d8
update test
mushao999 Aug 18, 2022
a6c4e25
add changelog
mushao999 Aug 18, 2022
51fcaf8
remove the changelog
mushao999 Sep 12, 2022
6bebd94
handle invalid phase
mushao999 Sep 12, 2022
8ae3668
Merge branch 'main' into optimize/MigrateAction
elasticmachine Sep 12, 2022
990f270
move invalid phase checking to method head
mushao999 Sep 12, 2022
ce2b0be
return false for invalid phase
mushao999 Sep 15, 2022
c96628e
Merge branch 'main' into optimize/MigrateAction
elasticmachine Sep 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,11 +154,20 @@ public List<Phase> getOrderedPhases(Map<String, Phase> phases) {
}

public static boolean shouldInjectMigrateStepForPhase(Phase phase) {
if (ALLOWED_ACTIONS.containsKey(phase.getName()) == false) {
return false;
}

// searchable snapshots automatically set their own allocation rules, no need to configure them with a migrate step.
if (phase.getActions().get(SearchableSnapshotAction.NAME) != null) {
return false;
}

// do not inject if MigrateAction is not supported for this phase (such as hot, frozen, delete phase)
if (ALLOWED_ACTIONS.get(phase.getName()).contains(MigrateAction.NAME) == false) {
return false;
}

Copy link
Contributor

@gmarouli gmarouli Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is public and can be called with an invalid phase as an argument, if that happens this will throw an NPE. I think we should check this and throw an IllegalArgumentException like we do in other methods in this class.

Copy link
Contributor Author

@mushao999 mushao999 Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I've updated the code in this way.

Copy link
Contributor

@gmarouli gmarouli Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about restructuring the conditionals like this:

  • We start the method with this:
if (ALLOWED_ACTIONS.containsKey(phase.getName()) == false) {
    throw new IllegalArgumentException("Timeseries lifecycle does not support phase [" + phase.getName() + "]");
}

This way we make sure that we have consistent behaviour every time there is an invalid phase. The way the code is now, if there are searchable snapshots in an invalid phase it will return false instead of IllegalArgumentException.

  • Then we could continue listing the phase independent cases, and finally just return if it's allowed in this phase or not:
// if the user configured the {@link MigrateAction} already we won't automatically configure it
if (phase.getActions().get(MigrateAction.NAME) != null) {
    return false;
}
        
// only inject the MigrateAction id it's supported for this phase (for example warm, cold).
return ALLOWED_ACTIONS.get(phase.getName()).contains(MigrateAction.NAME);

What do you think?

Copy link
Contributor Author

@mushao999 mushao999 Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put the invalid-phase-checking there, beacase I want to make the change as smaller as possible.
Moving this checking to the head of this method will change the behaviour of invalid phase who has SearchableSnapshotAction in this method. False would be returned before, but IllegalArgumentException will be thrown for now.
But as you said, we could make sure consistent behaviour for invalid phases in this way which I think is more important for long terms.
I've update the code in this way.

Copy link
Contributor

@gmarouli gmarouli Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I see, you are right we are changing the behaviour but I would expect if an invalid phase is provided that it would blow up anyway in a similar way since other methods do that too. I would like to ask another opinion, @dakrone do you see any red flags for throwing an IllegalArgumentException if we get an invalid phase?

Copy link
Contributor

@gmarouli gmarouli Sep 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank for joining the discussion @andreidan , in that case we would be throwing an NPE, right? Since it's a public method we cannot control that the phase has passed any check.

Copy link
Contributor

@andreidan andreidan Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmarouli that's correct. I missed we're actually adding the check that uses ALLOWED_ACTIONS.

I think it makes sense to have a check to avoid NPE. IMO returning false for an invalid phase would be preferable - ie. migrate should NOT be injected (as we're not really validating the allowed phases here), but an exception could work too.

Copy link
Contributor

@gmarouli gmarouli Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I am okay with returning false consistently when we receive a phase we do not recognize. @mushao999 if you agree too with this approach, let's replace the exception with returning false and let's ship it! 🚀

Copy link
Contributor Author

@mushao999 mushao999 Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmarouli It's ok for me, code updated.

Copy link
Contributor

@gmarouli gmarouli Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I will merge it with master asap.

// if the user configured the {@link MigrateAction} already we won't automatically configure it
if (phase.getActions().get(MigrateAction.NAME) != null) {
return false;
Expand Down
23 changes: 23 additions & 0 deletions ...gin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,29 @@ public void testShouldMigrateDataToTiers() {
assertThat(TimeseriesLifecycleType.shouldInjectMigrateStepForPhase(phase), is(false));
}

{
// not inject in hot phase
Phase phase = new Phase(HOT_PHASE, TimeValue.ZERO, Collections.emptyMap());
assertThat(TimeseriesLifecycleType.shouldInjectMigrateStepForPhase(phase), is(false));
}

{
// not inject in frozen phase
Phase phase = new Phase(FROZEN_PHASE, TimeValue.ZERO, Collections.emptyMap());
assertThat(TimeseriesLifecycleType.shouldInjectMigrateStepForPhase(phase), is(false));
}

{
// not inject in delete phase
Phase phase = new Phase(DELETE_PHASE, TimeValue.ZERO, Collections.emptyMap());
assertThat(TimeseriesLifecycleType.shouldInjectMigrateStepForPhase(phase), is(false));
}

{
// return false for invalid phase
Phase phase = new Phase(HOT_PHASE + randomAlphaOfLength(5), TimeValue.ZERO, Collections.emptyMap());
assertThat(TimeseriesLifecycleType.shouldInjectMigrateStepForPhase(phase), is(false));
}
}

public void testValidatingSearchableSnapshotRepos() {
Expand Down