[ML] Delete forecast API (#31134) by benwtrent · Pull Request #33218 · elastic/elasticsearch

benwtrent · 2018-08-28T21:39:20Z

Integration tests added to test the action. Verified that the API is published appropriately and works manually as well.

Addresses feature request: (#31134)

elasticmachine · 2018-08-28T21:39:22Z

Pinging @elastic/ml-core

dimitris-athanasiou

Looks good. Left some minor comments and a bigger one about whether we should handle deletion of multiple forecasts at once.

dimitris-athanasiou · 2018-08-29T12:07:05Z

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

    public static final String REST_START_AFTER_END = "Invalid time range: end time ''{0}'' is earlier than start time ''{1}''.";
-
+    public static final String REST_NO_SUCH_FORECAST = "No forecast with id [{0}] exists for job [{1}]";
+    public static final String REST_BAD_FORECAST_STATE = "Forecast [{0}] for job [{1}] needs to be either FAILED or FINISHED to be deleted";


The name could be more descriptive here. Something like REST_CANNOT_DELETE_FORECAST_IN_CURRENT_STATE.

dimitris-athanasiou · 2018-08-29T12:12:54Z

...plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteForecastAction.java

+                return;
+            }
+
+            if (DELETABLE_STATUSES.contains(forecastRequestStats.getStatus())) {


We could reduce the indentation here by checking if the status is not deletable first, then proceed in doing the deletion. I leave it on your preference whether you want to change it.

dimitris-athanasiou · 2018-08-29T12:18:41Z

...plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteForecastAction.java

+                DeleteByQueryRequest deleteByQueryRequest = buildDeleteByQuery(jobId, forecastId);
+                executeAsyncWithOrigin(client, ML_ORIGIN, DeleteByQueryAction.INSTANCE, deleteByQueryRequest, ActionListener.wrap(
+                    response -> {
+                        if (response.getDeleted() > 0) {


We should handle error paths here. We need to check if the request timed out and we need to check if we got any bulk failures. I noticed we don't do that from the expired data removers, but arguably we should also be doing better error checking in those.

dimitris-athanasiou · 2018-08-29T12:31:17Z

...ck/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/rest/job/RestDeleteForecastAction.java

+    @Override
+    protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient client) throws IOException {
+        String jobId = restRequest.param(Job.ID.getPreferredName());
+        String forecastId = restRequest.param(Forecast.FORECAST_ID.getPreferredName());


As the delete may take a while, we should be allowing for timeouts. See datafeed deletion as an example.

dimitris-athanasiou · 2018-08-29T12:33:11Z

...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/DeleteForecastAction.java

+    public static class Request extends ActionRequest {
+
+        private String jobId;
+        private String forecastId;


Contrary to the other delete requests we currently have, this one deletes results instead of resources. I can imagine the UI allowing users to select multiple forecasts and then delete them in bulk. It would be a pain for the UI to have to perform a request per forecast. I think we should consider allowing this to handle deletion of multiple forecasts. Users can pass a comma separated list of forecast IDs.

dimitris-athanasiou · 2018-08-29T12:38:02Z

...native-multi-node-tests/src/test/java/org/elasticsearch/xpack/ml/integration/ForecastIT.java


    }

+    public void testDelete() throws Exception {


We should also add a YML test in forecast.yml. Ping me if you need a tutorial on how those work.

dimitris-athanasiou · 2018-08-29T14:18:56Z

Also, don't forget to add an entry to add this API to the client once merged in.

benwtrent · 2018-08-29T14:42:27Z

@dimitris-athanasiou most definitely :)

dimitris-athanasiou · 2018-08-31T10:55:28Z

...plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteForecastAction.java

+            .minimumShouldMatch(1)
+            .must(QueryBuilders.termsQuery(Result.RESULT_TYPE.getPreferredName(), ForecastRequestStats.RESULT_TYPE_VALUE))
+            .should(QueryBuilders.boolQuery()
+                .must(QueryBuilders.termQuery(Job.ID.getPreferredName(), jobId))))


If the forecastId is not _all, I think we should add a terms query here with the requested forecast IDs. It makes this more efficient, it saves having to filter the forecast IDs later on and it dodges the 10K limit in the odd case.

dimitris-athanasiou · 2018-08-31T10:57:01Z

...plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteForecastAction.java

+            .should(QueryBuilders.boolQuery()
+                .must(QueryBuilders.termQuery(Job.ID.getPreferredName(), jobId))))
+            .size(MAX_FORECAST_TO_SEARCH);
+        SearchRequest searchRequest = new SearchRequest(RESULTS_INDEX_PATTERN);


Here we should set AnomalyDetectorsIndex.jobResultsAliasedName. The aliases contain a filter on the job_id which means we won't match forecasts of other jobs. This is important to dodge the 10K limit.

dimitris-athanasiou · 2018-08-31T11:02:22Z

...plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteForecastAction.java

+        ActionListener<SearchResponse> forecastStatsHandler = ActionListener.wrap(
+            searchResponse -> deleteForecasts(searchResponse, request, listener),
+            e -> listener.onFailure(new ElasticsearchException("An error occurred while searching forecasts to delete", e)));
+


If the forecast_id is _all, we need to check the cluster setting action.destructive_requires_name (see https://www.elastic.co/guide/en/elasticsearch/guide/current/_deleting_an_index.html). If that is true, we shouldn't allow _all.

dimitris-athanasiou · 2018-08-31T11:04:26Z

...plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteForecastAction.java

+            e -> listener.onFailure(new ElasticsearchException("An error occurred while searching forecasts to delete", e)));
+
+        SearchSourceBuilder source = new SearchSourceBuilder();
+        source.query(QueryBuilders.boolQuery()


The query here should be in a filter context (see https://www.elastic.co/guide/en/elasticsearch/reference/current/query-filter-context.html). When we are not interested in scoring, we should be using the filter context.

dimitris-athanasiou · 2018-08-31T11:04:58Z

...plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteForecastAction.java

+            .minimumShouldMatch(1)
+            .must(QueryBuilders.termsQuery(Result.RESULT_TYPE.getPreferredName(), ForecastRequestStats.RESULT_TYPE_VALUE))
+            .should(QueryBuilders.boolQuery()
+                .must(QueryBuilders.termQuery(Job.ID.getPreferredName(), jobId))))


This won't be necessary when we use the job results index alias (see below).

dimitris-athanasiou · 2018-08-31T11:11:40Z

...plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteForecastAction.java

+                        new TimeoutException("Unable to delete all requested forecasts. Deleted a total of " + response.getDeleted()));
+                    return;
+                }
+                if (response.getBulkFailures().isEmpty() && response.getSearchFailures().isEmpty()) {


This is checking there are no failures. It should be changed to check there are failures.

dimitris-athanasiou · 2018-08-31T11:14:28Z

...plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteForecastAction.java

+            .setSize(MAX_FORECAST_TO_SEARCH)
+            .setSlices(5);

        searchRequest.indices(RESULTS_INDEX_PATTERN);


Similarly, use job results index alias here.

dimitris-athanasiou · 2018-08-31T11:15:12Z

...plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteForecastAction.java

+            boolQuery.should(QueryBuilders.boolQuery()
                .must(QueryBuilders.termQuery(Job.ID.getPreferredName(), jobId))
-                .must(QueryBuilders.termQuery(Forecast.FORECAST_ID.getPreferredName(), forecastId)));
+                .must(QueryBuilders.termQuery(Forecast.FORECAST_ID.getPreferredName(), forecastToDelete)));


You can use a termsQuery instead where you pass all the IDs and you don't have to do the for loop.

dimitris-athanasiou · 2018-08-31T11:17:27Z

x-pack/plugin/src/test/resources/rest-api-spec/api/xpack.ml.delete_forecast.json

+        "allow_no_forecasts": {
+          "type": "boolean",
+          "required": false,
+          "description": "Whether to ignore if `_all` or `*` matches no forecasts"


also remove * from here.

dimitris-athanasiou · 2018-08-31T11:20:57Z

x-pack/plugin/src/test/resources/rest-api-spec/test/ml/delete_forecast.yml

+  - do:
+      headers:
+        Authorization: "Basic eF9wYWNrX3Jlc3RfdXNlcjp4LXBhY2stdGVzdC1wYXNzd29yZA==" # run as x_pack_rest_user, i.e. the test setup superuser
+      xpack.ml.post_data:


I don't see this setup being necessary for any of the tests. However, it would be nice to test the delete actually removes forecasts. You will need to index a forecast manually (a forecast stats doc plus a couple of forecast docs). We follow a similar approach in the YAML tests of the results.

dimitris-athanasiou · 2018-09-03T13:36:47Z

...plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteForecastAction.java

                }
-                if (response.getBulkFailures().isEmpty() && response.getSearchFailures().isEmpty()) {
+                if ((response.getBulkFailures().isEmpty() && response.getSearchFailures().isEmpty()) == false) {
+                    Tuple<RestStatus, Throwable> statusAndReason = getStatusAndReason(response);


We discussed that we should ignore version conflict exceptions as they imply the document has already been deleted. Won't those be included in the bulk failures? If yes, I don't see where we filter them out.

@dimitris-athanasiou I tested with ignore false and true. When the ignore is true, no exception is thrown due to version conflict, however, if the ignore is false, I do get an exception thrown. So, I don't think any filtering is necessary from my testing.

I added a test that fails when that field is false (as it does not expect an exception), and passes when true so that we can be alerted through regression if this behavior changes.

Which ignore are you referring to?

dimitris-athanasiou · 2018-09-03T13:38:22Z

...plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteForecastAction.java

-
-        if (MetaData.ALL.equals(forecastsExpression) || Regex.isMatchAllPattern(forecastsExpression)) {
-            return new HashSet<>(allStats);
+            XContentParser parser = XContentFactory.xContent(XContentType.JSON).createParser(


The parser should be created in a try-with-resource.

dimitris-athanasiou · 2018-09-03T13:44:44Z

Almost there! Left 2 more comments that for some reason github shows as outdated.

dimitris-athanasiou · 2018-09-03T15:20:08Z

...plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteForecastAction.java

-            XContentParser parser = XContentFactory.xContent(XContentType.JSON).createParser(
-                NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, hit.getSourceRef().streamInput());
-            allStats.add(ForecastRequestStats.STRICT_PARSER.apply(parser, null));
+            try (InputStream stream = hit.getSourceRef().streamInput();


Here, we don't need the stream in a separate variable. The parser owns it and will handle closing the stream.

dimitris-athanasiou

LGTM

* Delete forecast API (#31134)

This reverts commit e1b985e.

jasontedor · 2018-09-04T04:30:04Z

I reverted this commit from 6.x via bf01cda because it broke compilation there.

* Delete forecast API (elastic#31134)

…e-default-distribution * elastic/master: (213 commits) ML: Fix build after HLRC change Fix inner hits retrieval when stored fields are disabled (_none_) (elastic#33018) SQL: Show/desc commands now support table ids (elastic#33363) Mute testValidateFollowingIndexSettings HLRC: Add delete by query API (elastic#32782) [ML] The sort field on get records should default to the record_score (elastic#33358) [ML] Minor improvements to categorization Grok pattern creation (elastic#33353) [DOCS] fix a couple of typos (elastic#33356) Disable assemble task instead of removing it (elastic#33348) Simplify the return type of FieldMapper#parse. (elastic#32654) [ML] Delete forecast API (elastic#31134) (elastic#33218) Introduce private settings (elastic#33327) [Docs] Add search timeout caveats (elastic#33354) TESTS: Fix Race Condition in Temp Path Creation (elastic#33352) Fix from_range in search_after in changes snapshot (elastic#33335) TESTS+DISTR.: Fix testIndexCheckOnStartup Flake (elastic#33349) Null completion field should not throw IAE (elastic#33268) Adds code to help with IndicesRequestCacheIT failures (elastic#33313) Prevent NPE parsing the stop datafeed request. (elastic#33347) HLRC: Add ML get overall buckets API (elastic#33297) ...

* [ML] Delete forecast API (#31134) (#33218) * Delete forecast API (#31134) * Adjust for backport * removing bad import

* [ML] Delete forecast API (#31134) (#33218) * Delete forecast API (#31134) * Adjust for backport * removing bad import * Fixing delete forecast action

Delete forecast API (elastic#31134)

d40b201

benwtrent added >enhancement review v7.0.0 :ml Machine learning v6.5.0 labels Aug 28, 2018

dimitris-athanasiou reviewed Aug 29, 2018

View reviewed changes

Merge branch 'master' into feature/delete-forecast-api

ec318a3

dimitris-athanasiou changed the title ~~Delete forecast API (#31134)~~ [ML] Delete forecast API (#31134) Aug 29, 2018

benwtrent added 2 commits August 30, 2018 13:12

Adding ability to delete more than one forecast

c6d3869

minor changes and adding yaml tests

283f3fc

dimitris-athanasiou reviewed Aug 31, 2018

View reviewed changes

benwtrent added 3 commits August 31, 2018 11:07

Fixing yaml tests, adjusting search and delete

70cdfaf

Merge branch 'master' into feature/delete-forecast-api

96c53e1

Merge branch 'master' into feature/delete-forecast-api

cd7b37c

dimitris-athanasiou reviewed Sep 3, 2018

View reviewed changes

benwtrent added 2 commits September 3, 2018 10:15

making stream and parser try-with-resource

08a32bf

Merge branch 'master' into feature/delete-forecast-api

d170da4

dimitris-athanasiou reviewed Sep 3, 2018

View reviewed changes

dimitris-athanasiou approved these changes Sep 3, 2018

View reviewed changes

benwtrent added 3 commits September 3, 2018 14:13

Merge branch 'master' into feature/delete-forecast-api

de0bead

blacklisting tests that are meant to raise errors

eccab09

Removing forbidden API utilization

8643d1a

benwtrent merged commit 767d8e0 into elastic:master Sep 4, 2018

benwtrent deleted the feature/delete-forecast-api branch September 4, 2018 00:06

benwtrent added a commit that referenced this pull request Sep 4, 2018

[ML] Delete forecast API (#31134) (#33218)

e1b985e

* Delete forecast API (#31134)

jasontedor added a commit that referenced this pull request Sep 4, 2018

Revert "[ML] Delete forecast API (#31134) (#33218)"

bf01cda

This reverts commit e1b985e.

benwtrent added a commit to benwtrent/elasticsearch that referenced this pull request Sep 4, 2018

[ML] Delete forecast API (elastic#31134) (elastic#33218)

107207e

* Delete forecast API (elastic#31134)

benwtrent mentioned this pull request Sep 4, 2018

Feature/delete forecast api backport #33381

Merged

benwtrent added a commit that referenced this pull request Sep 4, 2018

Feature/delete forecast api backport (#33381)

282ab8f

* [ML] Delete forecast API (#31134) (#33218) * Delete forecast API (#31134) * Adjust for backport * removing bad import

lcawl mentioned this pull request Sep 4, 2018

[DOCS] Adds delete forecast API #33401

Merged

benwtrent added a commit that referenced this pull request Sep 5, 2018

Feature/delete forecast api backport (#33397)

0ac7ff8

* [ML] Delete forecast API (#31134) (#33218) * Delete forecast API (#31134) * Adjust for backport * removing bad import * Fixing delete forecast action

Mpdreamz mentioned this pull request Dec 13, 2018

[meta] 6.5.0 Release elastic/elasticsearch-net#3457

Closed

codebrain mentioned this pull request Jan 28, 2019

[meta] 6.6.0 Release elastic/elasticsearch-net#3552

Closed

48 tasks

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Conversation

benwtrent commented Aug 28, 2018

Uh oh!

elasticmachine commented Aug 28, 2018

Uh oh!

dimitris-athanasiou 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!

dimitris-athanasiou commented Aug 29, 2018

Uh oh!

benwtrent commented Aug 29, 2018

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!

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 commented Sep 3, 2018

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!

jasontedor commented Sep 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants