-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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.
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
Allow shrink in the hot phase for ILM policies #64008
Conversation
and lets us drop the static initializer
to allow the possibility that there could be more actions in the hot phase that require rollover, rather than only just forcemerge.
as long as there's an accompanying rollover.
Inline forceMergeActionWithCodec in place of its only caller.
Move forcemerge after rollover, since that's when it'll actually execute. Add shrink to the hot phase. Migrate was missing in the warm and cold phases, fix that.
Pinging @elastic/es-core-features (:Core/Features/ILM+SLM) |
@elasticmachine update branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I left one comment about a change we should undo though, other than that, looks good!
@@ -50,14 +50,13 @@ | |||
static final Set<String> VALID_WARM_ACTIONS = Sets.newHashSet(ORDERED_VALID_WARM_ACTIONS); | |||
static final Set<String> VALID_COLD_ACTIONS = Sets.newHashSet(ORDERED_VALID_COLD_ACTIONS); | |||
static final Set<String> VALID_DELETE_ACTIONS = Sets.newHashSet(ORDERED_VALID_DELETE_ACTIONS); | |||
private static final Map<String, Set<String>> ALLOWED_ACTIONS = new HashMap<>(); | |||
private static final Map<String, Set<String>> ALLOWED_ACTIONS = Map.of( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not compile on backport (which is fine, just wanted to let you know since you asked about this previously)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, I've pregamed that and I'm ready to do the backport dance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have these utility classes in the 7.x
branch that provide these functions in a Java8-compatible way so that backports are easier. They're also packaged as a multi-release JAR so that in JVMs that are 9 or later, they use the Java libraries instead, so there's no downside to using them when running with later JVMs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danhermann the utility classes are unfortunately not that helpful, since to use they the Map.of
needs to be fully qualified, or java.util.Map<String, Set<String>>
has to be specified to fully qualify the jdk classes, which I think is more awkward than just changing on backport.
...lti-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java
Outdated
Show resolved
Hide resolved
This reverts commit fe5c390.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @joegallo
LGTM
|
Resolves #56377, very much along the same lines as #52073
This PR changes the shrink action to also be allowed in the hot phase after a rollover. As with #52073, a shrink in the hot phase MUST be accompanied by a rollover, and policy validation has been updated to check for this.
Reviewing commit-by-commit is best, there's a few relatively self-contained cleanup commits.