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
[Data stream lifecycle]Add warning when the user's retention is not the effective retention #107781
Conversation
Pinging @elastic/es-data-management (Team:Data Management) |
server/src/main/java/org/elasticsearch/cluster/metadata/DataStreamLifecycle.java
Outdated
Show resolved
Hide resolved
...r/src/test/java/org/elasticsearch/cluster/metadata/DataStreamLifecycleWithWarningsTests.java
Outdated
Show resolved
Hide resolved
...r/src/test/java/org/elasticsearch/cluster/metadata/DataStreamLifecycleWithWarningsTests.java
Outdated
Show resolved
Hide resolved
|
||
DataStreamLifecycle noRetentionLifecycle = DataStreamLifecycle.newBuilder().downsampling(randomDownsampling()).build(); | ||
DataStreamGlobalRetention globalRetention = new DataStreamGlobalRetention( | ||
randomTimeValue(2, 10, TimeUnit.DAYS), |
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.
Extremely minor nit: you sometimes use randomTimeValue(2, 10, TimeUnit.DAYS)
and sometimes TimeValue.timeValueDays(randomIntBetween(5, 100))
. I think it wouldn't hurt to stick to one for consistency's sake :). The former is slightly shorter than the latter, so maybe I have the tiniest preference for that, but I'll let it up to you 😉.
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.
You are right, and it's not an inconsistency, just trying to keep the PR manageable. randomTimeValue(2, 10, TimeUnit.DAYS)
was recently introduced, so I use it now in the code I touch. Then in a separate PR I will change it because otherwise we are going to touch many other files.
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.
Ah okay that makes sense. I did notice one in this PR here, that's the only reason I mentioned it :).
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataDataStreamsService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/node/NodeConstruction.java
Outdated
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 for the iterations Mary :)
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.
construction changes look good
@elasticmachine update branch |
@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.
Looks good to me!
@elasticmachine update branch |
We want to inform the user when the retention provided in a data stream or a template is not going to be the effective retention.
We choose not to reject the configuration because:
Additionally, we add a warning header to inform the user about the effective retention.