Skip to content

[ML] Use results retention time for deleting system annotations #76096

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

Merged

Conversation

droberts195
Copy link
Contributor

In #75617 a new setting, system_annotations_retention_days, was
added to control how long system annotations are retained for.
We now feel that this setting is redundant and that system
annotations should be retained for the same period as results.
This is intuitive and defensible, as system annotations can be
considered a type of result.

Followup to #75617

In elastic#75617 a new setting, system_annotations_retention_days, was
added to control how long system annotations are retained for.
We now feel that this setting is redundant and that system
annotations should be retained for the same period as results.
This is intuitive and defensible, as system annotations can be
considered a type of result.

Followup to elastic#75617
@elasticmachine elasticmachine added the Team:ML Meta label for the ML team label Aug 4, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

@droberts195
Copy link
Contributor Author

>non-issue because the original change was never released.

Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

very minor things, but I like the change. Keep it simple!

@@ -1433,16 +1433,6 @@ bucket result are deleted from {es}. The default value is null, which means all
results are retained.
Copy link
Member

Choose a reason for hiding this comment

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

We should comment here that system created annotations are also deleted.

@@ -193,7 +192,9 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeOptionalLong(modelSnapshotRetentionDays);
out.writeOptionalLong(dailyModelSnapshotRetentionAfterDays);
out.writeOptionalLong(resultsRetentionDays);
out.writeOptionalLong(systemAnnotationsRetentionDays);
if (out.getVersion().onOrAfter(Version.V_7_15_0) && out.getVersion().before(Version.V_8_0_0)) {
out.writeOptionalLong(null);
Copy link
Member

Choose a reason for hiding this comment

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

I think technically this should be out.writeOptionalLong(resultsRetentionDays);

But, that doesn't matter really as when this change is backported this branch is removed.

@droberts195 droberts195 merged commit 7ac5ea3 into elastic:master Aug 4, 2021
@droberts195 droberts195 deleted the use_results_retention_for_annotations branch August 4, 2021 16:42
droberts195 added a commit to droberts195/elasticsearch that referenced this pull request Aug 4, 2021
Remove the last bits of code after the backport PR (elastic#76113)
is merged.

Followup to elastic#76096
benwtrent added a commit to benwtrent/elasticsearch that referenced this pull request Aug 4, 2021
elasticsearchmachine pushed a commit that referenced this pull request Aug 4, 2021
* [ML] Use results retention time for deleting system annotations

In #75617 a new setting, system_annotations_retention_days, was
added to control how long system annotations are retained for.
We now feel that this setting is redundant and that system
annotations should be retained for the same period as results.
This is intuitive and defensible, as system annotations can be
considered a type of result.

Backport of #76096

* Fix one more merge clash
elasticsearchmachine pushed a commit that referenced this pull request Aug 4, 2021
* [ML] Removing last traces of system_annotations_retention_days

Remove the last bits of code after the backport PR (#76113)
is merged.

Followup to #76096

* unmute bwc tests

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Benjamin Trent <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml Machine learning >non-issue Team:ML Meta label for the ML team v7.15.0 v8.0.0-alpha2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants