Fix delete_expired_data/nightly maintenance when many model snapshots need deleting #57041

davidkyle · 2020-05-21T13:39:26Z

The queries performed by the expired data removers pull back entire documents where only a few fields are required. For ModelSnapshots in particular this is a problem as they contain quantiles which may be 100s of KB and the search size is set to 10,000.

If the user is suffering with many accumulated snapshots that were not cleaned up due to #47103 the size of this search response could be very large. This change makes the search more efficient by only requesting the fields needed to work out which expired data should be deleted.

elasticmachine · 2020-05-21T13:39:28Z

Pinging @elastic/ml-core (:ml)

droberts195

LGTM

All my comments on the code are minor nits.

My main comment is that we should consider this a bug rather than an enhancement and backport it to 7.8.0 and 7.7.1. From the perspective of an end user who is suffering from this the product doesn't work.

droberts195 · 2020-05-21T13:46:22Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/extractor/TimeField.java

-            } else {
-                value[0] = 0L;
-            }
+           value[0] = TimeUtils.parseToEpochMs((String)value[0]);


nit: indenting is out by 1 space by the look of it

droberts195 · 2020-05-21T13:48:11Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/common/time/TimeUtils.java

+     * @param epoch The
+     * @return The epoch value.
+     */
+    public static Long parseToEpochMs(String epoch) {


Would it be better to return long? In the case where the caller wants Long it can be boxed just as efficiently on the caller's side. And if the caller wants long then there's no boxing and unboxing.

droberts195 · 2020-05-21T13:51:38Z

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

-                        long timestampMs = modelSnapshot.getTimestamp().getTime();
+                        String timestamp = stringFieldValueOrNull(hit, ModelSnapshot.TIMESTAMP.getPreferredName());
+                        if (timestamp == null) {
+                            continue;


Maybe we should log an error here, as I don't think this should never happen unless a document is corrupt. So if it does happen it would be useful to know about when debugging why results haven't been deleted.

droberts195 · 2020-05-21T13:52:28Z

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

+                        if (timestamp == null) {
+                            continue;
+                        }
+                        Long timestampMs = TimeUtils.parseToEpochMs(timestamp);


Could use long instead of Long and avoid unboxing in the loop.

droberts195 · 2020-05-21T14:01:51Z

I changed the PR title so affected users can find it more easily in the release notes.

przemekwitek · 2020-05-21T13:56:02Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/common/time/TimeUtils.java

+     */
+    public static Long parseToEpochMs(String epoch) {
+        int dotPos = epoch.indexOf('.');
+        long epochMs = 0L;


Alternatively, you could get rid of epochMs and use return statement in each of 3 if branches.

Yes that does read better

przemekwitek · 2020-05-21T13:57:14Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/common/time/TimeUtils.java

+     * JSON.  So String is really the only format that could be used, but the consumers of time
+     * are expecting a number.
+     *
+     * @param epoch The


Please write sth here.

droberts195 · 2020-05-21T15:58:31Z

...lugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/retention/ExpiredForecastsRemover.java

@@ -84,7 +78,11 @@ public void remove(float requestsPerSec, ActionListener<Boolean> listener, Suppl
                .filter(QueryBuilders.termQuery(Result.RESULT_TYPE.getPreferredName(), ForecastRequestStats.RESULT_TYPE_VALUE))
                .filter(QueryBuilders.existsQuery(ForecastRequestStats.EXPIRY_TIME.getPreferredName())));
        source.size(MAX_FORECASTS);
-        source.trackTotalHits(true);


Will removing this make:

if (hits.getTotalHits().value > MAX_FORECASTS) { LOGGER.info("More than [{}] forecasts were found. This run will only delete [{}] of them", MAX_FORECASTS, MAX_FORECASTS); }

less useful? (Around line 141/145.)

Good spot I couldn't see where it was checked so removed it.

The benefit of setting trackTotalHits(true) as the only purpose is to log this message saying there are more forecasts to be deleted later. However, in the interest of making this change as small as possible I've reverted this line

… need deleting (elastic#57041) The queries performed by the expired data removers pull back entire documents when only a few fields are required. For ModelSnapshots in particular this is a problem as they contain quantiles which may be 100s of KB and the search size is set to 10,000. This change makes the search more efficient by only requesting the fields needed to work out which expired data should be deleted.

…pshots need deleting (#57041) (#57136) Fix delete_expired_data/nightly maintenance when many model snapshots need deleting (#57041) The queries performed by the expired data removers pull back entire documents when only a few fields are required. For ModelSnapshots in particular this is a problem as they contain quantiles which may be 100s of KB and the search size is set to 10,000. This change makes the search more efficient by only requesting the fields needed to work out which expired data should be deleted.

… need deleting (elastic#57041) The queries performed by the expired data removers pull back entire documents when only a few fields are required. For ModelSnapshots in particular this is a problem as they contain quantiles which may be 100s of KB and the search size is set to 10,000. This change makes the search more efficient by only requesting the fields needed to work out which expired data should be deleted. # Conflicts: # x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/retention/ExpiredForecastsRemover.java # x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/retention/ExpiredModelSnapshotsRemover.java # x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/retention/MlDataRemover.java # x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/retention/ExpiredModelSnapshotsRemoverTests.java

… need deleting (elastic#57041) The queries performed by the expired data removers pull back entire documents when only a few fields are required. For ModelSnapshots in particular this is a problem as they contain quantiles which may be 100s of KB and the search size is set to 10,000. This change makes the search more efficient by only requesting the fields needed to work out which expired data should be deleted.

… need deleting (#57041) (#57145) The queries performed by the expired data removers pull back entire documents when only a few fields are required. For ModelSnapshots in particular this is a problem as they contain quantiles which may be 100s of KB and the search size is set to 10,000. This change makes the search more efficient by only requesting the fields needed to work out which expired data should be deleted.

…pshots need deleting (#57041) (#57146) The queries performed by the expired data removers pull back entire documents when only a few fields are required. For ModelSnapshots in particular this is a problem as they contain quantiles which may be 100s of KB and the search size is set to 10,000. This change makes the search more efficient by only requesting the fields needed to work out which expired data should be deleted.

Extract time extractor method

e5becf8

davidkyle added >enhancement :ml Machine learning v8.0.0 v7.9.0 labels May 21, 2020

droberts195 approved these changes May 21, 2020

View reviewed changes

droberts195 changed the title ~~Request required fields only in expired data deleters~~ Fix delete_expired_data/nightly maintenance when many model snapshots need deleting May 21, 2020

droberts195 added >bug and removed >enhancement labels May 21, 2020

przemekwitek reviewed May 21, 2020

View reviewed changes

Don't get source

a69f90a

davidkyle force-pushed the required-fields-only branch from 8a7366d to a69f90a Compare May 21, 2020 15:33

droberts195 reviewed May 21, 2020

View reviewed changes

droberts195 added the v7.8.0 label May 21, 2020

Revert to tracking total hits

60936c0

davidkyle merged commit cde2026 into elastic:master May 26, 2020

davidkyle deleted the required-fields-only branch May 26, 2020 08:25

davidkyle mentioned this pull request May 26, 2020

[7.x] Fix delete_expired_data/nightly maintenance when many model snapshots need deleting (#57041) #57136

Merged

davidkyle mentioned this pull request May 26, 2020

[7.7.1] Fix delete_expired_data/nightly maintenance when many model snapshots… #57145

Merged

davidkyle added the v7.7.1 label May 26, 2020

davidkyle mentioned this pull request May 26, 2020

[7.8] Fix delete_expired_data/nightly maintenance when many model snapshots need deleting (#57041) #57146

Merged

davidkyle mentioned this pull request May 26, 2020

[6.8] Fix delete_expired_data/nightly maintenance when many model snapshots need deleting #57174

Merged

davidkyle added the v6.8.10 label May 27, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix delete_expired_data/nightly maintenance when many model snapshots need deleting #57041

Fix delete_expired_data/nightly maintenance when many model snapshots need deleting #57041

davidkyle commented May 21, 2020 •

edited

Loading

elasticmachine commented May 21, 2020

droberts195 left a comment

droberts195 May 21, 2020

droberts195 May 21, 2020

droberts195 May 21, 2020

droberts195 May 21, 2020

droberts195 commented May 21, 2020

przemekwitek May 21, 2020

davidkyle May 21, 2020

przemekwitek May 21, 2020

davidkyle May 21, 2020

droberts195 May 21, 2020

davidkyle May 21, 2020

Fix delete_expired_data/nightly maintenance when many model snapshots need deleting #57041

Fix delete_expired_data/nightly maintenance when many model snapshots need deleting #57041

Conversation

davidkyle commented May 21, 2020 • edited Loading

elasticmachine commented May 21, 2020

droberts195 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droberts195 commented May 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidkyle commented May 21, 2020 •

edited

Loading