-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Add max_single_primary_size as a condition for the ILM rollover action #68917
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
Add max_single_primary_size as a condition for the ILM rollover action #68917
Conversation
f05b75a
to
398d7c1
Compare
398d7c1
to
66b94d3
Compare
Pinging @elastic/es-core-features (Team:Core/Features) |
IMHO reviewing commit by commit would be best, but that's just me. |
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 Joe. This generally looks great! Nicely done.
I left a few rather minor suggestions and questions.
this.maxSize = maxSize; | ||
this.maxSinglePrimarySize = maxSinglePrimarySize; |
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.
would it make sense to validate that maxSinglePrimarySize
is lte to maxSize
?
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.
IMHO no -- we don't sanity check the other things (e.g. maxDocs
1 paired with maxSize
10,000GB is just as valid as maxDocs
10,000 with maxSize
1 byte -- at least AFAICT), so I think we should be similarly lenient with maxSinglePrimarySize
.
...lti-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java
Outdated
Show resolved
Hide resolved
...lti-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java
Show resolved
Hide resolved
...lti-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java
Outdated
Show resolved
Hide resolved
...lti-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java
Show resolved
Hide resolved
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, thanks Joe
@elastic/es-ui there's a new option for ILM Rollovers (in 7.13 and 8.0.0). On this PR it was added as |
Related to #63026, followup to #67842
Threads the new
max_single_primary_size
rollover trigger from #67842 into ILM'sRolloverAction
.