Add min_* conditions to rollover #83345
Conversation
|
@elasticsearchmachine run elasticsearch-ci/part-2 (failure unrelated, fixed by #83348) |
Unlike with an ILM rollover action, a max_* condition is not strictly required -- an unconditional rollover is valid. However, if any conditions are present, then at least one of them must be a max_* condition.
| @@ -67,6 +67,10 @@ public String name() { | |||
| return name; | |||
| } | |||
|
|
|||
| public boolean isRequired() { | |||
There was a problem hiding this comment.
Can you add javadocs for what this method is used for?
There was a problem hiding this comment.
I also think it's kind of a difficult/confusing name, perhaps "requiresAdditionalConditions" or "requiresMaxCondition" instead?
There was a problem hiding this comment.
How about inverting this -- rather than a boolean method that says "here's meaning" how about returning an Enum for "here's what this is"? That is, something like Condition.Type.MIN and Condition.Type.MAX.
We care about this in two places -- validate() (has to have at least one MAX), and also when evaluating areConditionsMet (all MIN conditions and at least one MAX condition must be 'met'). It seems like both those cases would read clearer if they just were what they said, rather than abstracting away behind some boolean method.
It'd be more direct, but it would also be tighter binding. That said, though, I don't see any reason we couldn't revisit that in the future if some third kind of condition or new rules, etc, were added.
There was a problem hiding this comment.
That would be totally okay with me too, my main concern is that from the name alone, there is no way to tell what this actually signifies, or how it is used. Returning a condition type would work for differentiating both of those, so that works for me.
server/src/main/java/org/elasticsearch/action/admin/indices/rollover/MinAgeCondition.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/admin/indices/rollover/MinAgeCondition.java
Outdated
Show resolved
Hide resolved
.../main/java/org/elasticsearch/action/admin/indices/rollover/MinPrimaryShardDocsCondition.java
Outdated
Show resolved
Hide resolved
.../main/java/org/elasticsearch/action/admin/indices/rollover/MinPrimaryShardDocsCondition.java
Outdated
Show resolved
Hide resolved
.../main/java/org/elasticsearch/action/admin/indices/rollover/MinPrimaryShardSizeCondition.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/admin/indices/rollover/MinSizeCondition.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/admin/indices/rollover/RolloverRequest.java
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/RolloverAction.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/RolloverAction.java
Outdated
Show resolved
Hide resolved
|
@joegallo according to this PR's labels, I need to update the changelog YAML, but I can't because the PR is closed. Please either update the changelog yourself on the appropriate branch, or adjust the labels. Specifically:
|
This change adds
min_age,min_size,min_docs,min_primary_shard_size, andmin_primary_shard_docsto the rollover API and ILM rollover action.Logic for determining if rollover should proceed:
min_*conditions must be met (AND logic) and at least onemax_*condition must be met (OR logic, the same as currently implemented).Note that the above means that if you were to define only
min_*conditions it will never rollover (since at least onemax_condition is required), therefore we validate for this explicitly in the ILM rollover action, as well as on the on rollover API itself.