[ML] optimize delete expired snapshots #63134

benwtrent · 2020-10-01T15:42:52Z

When deleting expired snapshots, we do an individual delete action per snapshot per job.

We should instead gather the expired snapshots and delete them in a single call.

This commit achieves this and a side-effect is there is less audit log spam on nightly cleanup

When deleting expired snapshots, we do an individual delete action per snapshot per job. We should instead gather the expired snapshots and delete them in a single call. This commit achieves this and a side-effect is there is less audit log spam on nightly cleanup closes elastic#62875

elasticmachine · 2020-10-01T15:42:54Z

Pinging @elastic/ml-core (:ml)

droberts195

Looks good. I just had one thought about the potential length of the audit message.

droberts195 · 2020-10-01T16:12:24Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java

    public static final String JOB_AUDIT_SNAPSHOT_STORED = "Job model snapshot with id [{0}] stored";
    public static final String JOB_AUDIT_REVERTED = "Job model snapshot reverted to ''{0}''";
    public static final String JOB_AUDIT_SNAPSHOT_DELETED = "Model snapshot [{0}] with description ''{1}'' deleted";
+    public static final String JOB_AUDIT_SNAPSHOTS_DELETED = "Model snapshots {0} with descriptions {1} deleted";


Maybe we could just say {0} expired model snapshots deleted (where {0} is how many). We could be deleting 24 per day if they've been changed to be every hour, and that would make the message very verbose. The fact that these are the ones that have expired should be OK for the audit log.

@droberts195 diggin' it

droberts195 · 2020-10-01T16:20:26Z

.../ml/src/main/java/org/elasticsearch/xpack/ml/job/retention/ExpiredModelSnapshotsRemover.java

-                }
-            });
+                auditor.info(jobId, msg);
+                LOGGER.debug(() -> new ParameterizedMessage("[{}] {}", jobId, msg));


Even though the audit message can be made less verbose, the debug should continue to list every ID and description, just in case something ever goes wrong with which snapshots are getting deleted.

benwtrent · 2020-10-01T18:14:58Z

...rc/test/java/org/elasticsearch/xpack/ml/job/retention/ExpiredModelSnapshotsRemoverTests.java

-        searchResponses.add(
-            AbstractExpiredJobDataRemoverTests.createSearchResponseFromHits(Collections.singletonList(snapshotToBeDeleted)));
-
-        SearchHit snapshot2_1 = createModelSnapshotQueryHit("job-1", "snapshots-1_1", eightDaysAndOneMsAgo);


I am not sure how this test ever passed?

I think it is meant to test the mixed retention between two jobs, but it appears to give the same job two of the snapshots?

I adjusted it it for the new format so that this other old snapshot is part of job-2

It's one of those tests where due to the complexity of the mocking it's very unclear what production code is being tested, and what's really being tested is the test itself.

The idea before was that job two had a 17 day retention period, so the search we were doing directly against the results index (without calling the model snapshots endpoint) would return nothing. This was represented by searchResponses.add(AbstractExpiredJobDataRemoverTests.createSearchResponseFromHits(Collections.emptyList())); in the old code.

@droberts195 I will update my mocks so that for job-2 nothing is returned

benwtrent · 2020-10-01T18:15:51Z

.../ml/src/main/java/org/elasticsearch/xpack/ml/job/retention/ExpiredModelSnapshotsRemover.java

-        client.execute(SearchAction.INSTANCE, searchRequest, new ThreadedActionListener<>(LOGGER, threadPool,
-            MachineLearning.UTILITY_THREAD_POOL_NAME, expiredSnapshotsListener(job.getId(), deleteAllBeforeMs, listener), false));
+        ActionListener<QueryPage<ModelSnapshot>> snapshotsListener = expiredSnapshotsListener(job, deleteAllBeforeMs, listener);
+        jobResultsProvider.modelSnapshots(job.getId(),


I was running into async callback hell. But, if we just use the underlying modelSnapshots request from here and then filter out the assigned snapshot later, we reduce the number of calls and it flows more cleanly.

benwtrent · 2020-10-01T18:16:08Z

.../ml/src/main/java/org/elasticsearch/xpack/ml/job/retention/ExpiredModelSnapshotsRemover.java

-                            LOGGER.warn("Model snapshot document [{}] has a null timestamp field", hit.getId());
+                    List<ModelSnapshot> snapshots = new ArrayList<>();
+                    for (ModelSnapshot snapshot: searchResponse.results()) {
+                        if (snapshot.getSnapshotId().equals(job.getModelSnapshotId())) {


Since we have to worry about the currently used model snapshot.

dimitris-athanasiou

LGTM2 Much nicer!

benwtrent · 2020-10-02T12:28:36Z

run elasticsearch-ci/bwc

benwtrent · 2020-10-02T12:50:50Z

run elasticsearch-ci/bwc

droberts195

I remember from when I put in the thinning out functionality what a nightmare those unit tests are because of the amount of mocking that is used.

Now that the production code has diverged even further from what it was when they were first written I am not convinced that they'd actually find a production problem any more.

I left a couple of comments about why one of them will be mega confusing for future maintainers if left as-is. The other two are also setting up weird responses that don't make sense given the settings, but they were doing this before your changes and they're testing error conditions. Maybe just add a comment to the mock response setup in those two saying the responses are wrong but OK given what's really being tested is the handling of unrelated errors.

The real coverage of this change comes from ModelSnapshotRetentionIT and DeleteExpiredDataIT. One of the tests in the latter is muted, although it was only failing very intermittently: DeleteExpiredDataIT.testDeleteExpiredDataWithStandardThrottle. Please can you try unmuting that and running it on your laptop and confirm it doesn't fail every time with these changes. (If it still passes almost every time then there's no need to fix the intermittent failure as part of this PR - just leave it muted in the committed code.)

droberts195 · 2020-10-02T12:20:17Z

...rc/test/java/org/elasticsearch/xpack/ml/job/retention/ExpiredModelSnapshotsRemoverTests.java

-        searchResponses.add(
-            AbstractExpiredJobDataRemoverTests.createSearchResponseFromHits(Collections.singletonList(snapshotToBeDeleted)));
-
-        SearchHit snapshot2_1 = createModelSnapshotQueryHit("job-1", "snapshots-1_1", eightDaysAndOneMsAgo);


It's one of those tests where due to the complexity of the mocking it's very unclear what production code is being tested, and what's really being tested is the test itself.

The idea before was that job two had a 17 day retention period, so the search we were doing directly against the results index (without calling the model snapshots endpoint) would return nothing. This was represented by searchResponses.add(AbstractExpiredJobDataRemoverTests.createSearchResponseFromHits(Collections.emptyList())); in the old code.

droberts195 · 2020-10-02T12:34:14Z

...rc/test/java/org/elasticsearch/xpack/ml/job/retention/ExpiredModelSnapshotsRemoverTests.java

+                createModelSnapshot("job-1", "active", oneDayAgo),
+                createModelSnapshot("job-1", "fresh-snapshot", oneDayAgo),


The model snapshots function shouldn't be returning snapshots from one day ago because the retention period is 7 days and a cutoff was supplied in https://github.com/elastic/elasticsearch/pull/63134/files#diff-496e9b129f2db0ef429636501b6974baR178

I think it's working because the thinning out functionality is ignoring the extra ones. But I don't think the code path being tested here is the code path that would get executed in production with the configured settings.

. But I don't think the code path being tested here is the code path that would get executed in production with the configured settings.

I understand. Including it is just a mocking mistake and the actual code path of "return only things before this timestamp" is not exercised. I could capture the query to check.

But I think the proof of this thing working is in the true integration tests (I will unmute the one you mentioned)

I will update the mocks

But I do think it is nice to test that active is not removed (since we are not filtering it in the query anylonger). I will make it 8days old for job-1. An admittedly rare case, but a good one to cover.

Yes, it's good to test active. Good idea to make that old enough that it would have been deleted.

droberts195 · 2020-10-02T12:34:44Z

...rc/test/java/org/elasticsearch/xpack/ml/job/retention/ExpiredModelSnapshotsRemoverTests.java

+                createModelSnapshot("job-2", "active", oneDayAgo),
+                createModelSnapshot("job-2", "snapshots-1_1", eightDaysAndOneMsAgo),
+                createModelSnapshot("job-2", "fresh-snapshot", oneDayAgo)


The model snapshots function shouldn't be returning snapshots from one or eight days ago because the retention period is 17 days and a cutoff was supplied in https://github.com/elastic/elasticsearch/pull/63134/files#diff-496e9b129f2db0ef429636501b6974baR178

OK, this is a mocking mistake. The change will not change the results of test.

It is nice to know though that even if the query is "wrong" that returns the snapshots, they are still not deleted :)

…lete-log-spam

droberts195

LGTM

When deleting expired snapshots, we do an individual delete action per snapshot per job. We should instead gather the expired snapshots and delete them in a single call. This commit achieves this and a side-effect is there is less audit log spam on nightly cleanup closes elastic#62875 # Conflicts: # x-pack/plugin/ml/qa/native-multi-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/ml/integration/DeleteExpiredDataIT.java # x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/retention/ExpiredModelSnapshotsRemover.java

* [ML] optimize delete expired snapshots (#63134) When deleting expired snapshots, we do an individual delete action per snapshot per job. We should instead gather the expired snapshots and delete them in a single call. This commit achieves this and a side-effect is there is less audit log spam on nightly cleanup closes #62875

benwtrent added >refactoring :ml Machine learning v8.0.0 v7.10.0 labels Oct 1, 2020

droberts195 reviewed Oct 1, 2020

View reviewed changes

benwtrent added 2 commits October 1, 2020 12:41

addressing pr comments

371c61b

addressing asynchronicity and reducing number of queries

474670f

benwtrent commented Oct 1, 2020

View reviewed changes

dimitris-athanasiou approved these changes Oct 2, 2020

View reviewed changes

benwtrent added 2 commits October 2, 2020 07:41

respect retain

49e5ea9

fixing sort order

5f70283

benwtrent requested a review from droberts195 October 2, 2020 12:03

droberts195 reviewed Oct 2, 2020

View reviewed changes

benwtrent added 2 commits October 2, 2020 10:02

adjusting tests

1bdb6b5

Merge remote-tracking branch 'upstream/master' into feature/ml-fix-de…

f903c6a

…lete-log-spam

droberts195 approved these changes Oct 2, 2020

View reviewed changes

benwtrent merged commit 4cfb22e into elastic:master Oct 2, 2020

benwtrent deleted the feature/ml-fix-delete-log-spam branch October 2, 2020 15:23

benwtrent mentioned this pull request Oct 2, 2020

[7.x] [ML] optimize delete expired snapshots (#63134) #63200

Merged

tvernum mentioned this pull request Oct 7, 2020

[CI] Suite Timeout in ML DeleteExpiredDataIT #63379

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

		createModelSnapshot("job-1", "active", oneDayAgo),
		createModelSnapshot("job-1", "fresh-snapshot", oneDayAgo),

[ML] optimize delete expired snapshots #63134

[ML] optimize delete expired snapshots #63134

Uh oh!

Conversation

benwtrent commented Oct 1, 2020

Uh oh!

elasticmachine commented Oct 1, 2020

Uh oh!

droberts195 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimitris-athanasiou left a comment

Choose a reason for hiding this comment

Uh oh!

benwtrent commented Oct 2, 2020

Uh oh!

benwtrent commented Oct 2, 2020

Uh oh!

droberts195 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droberts195 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants