[ML][Data Frame] responding with 409 status code when failing _stop #44231

benwtrent · 2019-07-11T16:08:31Z

When attempting a _stop call against any number of tasks with force=false and any of the tasks are failed, we should not stop any of the tasks. The response code should indicate an error with details in the body.

closes #44103

… _stop

elasticmachine · 2019-07-11T16:08:33Z

Pinging @elastic/ml-core

.../src/test/java/org/elasticsearch/xpack/dataframe/integration/DataFrameTaskFailedStateIT.java

davidkyle

Looks good. Left a few comments I don't think you need the integration test this is mergeable without it

...ain/java/org/elasticsearch/xpack/dataframe/action/TransportStopDataFrameTransformAction.java

.../src/test/java/org/elasticsearch/xpack/dataframe/integration/DataFrameTaskFailedStateIT.java

davidkyle · 2019-07-11T16:38:18Z

...ain/java/org/elasticsearch/xpack/dataframe/action/TransportStopDataFrameTransformAction.java

@@ -108,6 +134,8 @@ protected void taskOperation(StopDataFrameTransformAction.Request request, DataF
        }

        if (ids.contains(transformTask.getTransformId())) {
+            // This should not occur as we validate that none of the tasks are in a failed state earlier
+            // Keep this check in here for insurance.


davidkyle · 2019-07-11T16:40:56Z

...ain/java/org/elasticsearch/xpack/dataframe/action/TransportStopDataFrameTransformAction.java

@@ -88,8 +91,31 @@ protected void doExecute(Task task, StopDataFrameTransformAction.Request request
                new PageParams(0, 10_000),
                request.isAllowNoMatch(),
                ActionListener.wrap(hitsAndIds -> {
+                    if (request.isForce() == false) {
+                        PersistentTasksCustomMetaData tasks = state.metaData().custom(PersistentTasksCustomMetaData.TYPE);


This logic is the main change. If you really want to test this code extract it to a static method and it is easily unit testable. MlTasksTests is a good reference for building PersistentTasksCustomMetaData

...ain/java/org/elasticsearch/xpack/dataframe/action/TransportStopDataFrameTransformAction.java

benwtrent · 2019-07-11T18:30:52Z

run elasticsearch-ci/bwc
run elasticsearch-ci/default-distro

benwtrent · 2019-07-11T20:04:35Z

@elasticmachine update branch

hendrikmuhs · 2019-07-12T06:07:09Z

...tests/src/test/java/org/elasticsearch/xpack/dataframe/integration/DataFrameRestTestCase.java

@@ -161,7 +161,8 @@ protected void createContinuousPivotReviewsTransform(String transformId, String

        String config = "{ \"dest\": {\"index\":\"" + dataFrameIndex + "\"},"
            + " \"source\": {\"index\":\"" + REVIEWS_INDEX_NAME + "\"},"
-            + " \"sync\": {\"time\":{\"field\": \"timestamp\", \"delay\": \"15m\"}},"
+            //Set frequency high for testing


nit: "Set frequency high" - DYM low? The default is 60s, so 1s is lower?

High frequency => low time between repeats

(Similar to high frequency => short wavelength)

But if it's confusing then maybe bypass this by changing the comment to something like // Set frequency to 1s to make the test run as fast as possible.

This is one of those pedantic things that annoy me; frequency is measured in Hz not seconds, this is an interval. However, most people will read this and understand that frequency represents the period here and is probably the least confusing naming option.

A high frequency in Hz means a short distance between cycles.

A high frequency here means a short time between attempts.

hendrikmuhs · 2019-07-12T06:09:43Z

...ain/java/org/elasticsearch/xpack/dataframe/action/TransportStopDataFrameTransformAction.java

+                        failedTasks.get(0),
+                        failedReasons.get(0)) :
+                    "Unable to stop data frame transforms. The following transforms are in a failed state " +
+                        failedTasks + " with reasons " + failedReasons + ". Use force stop to stop the data frame transforms.";


nit: use a constant

hendrikmuhs · 2019-07-12T06:14:07Z

...ain/java/org/elasticsearch/xpack/dataframe/action/TransportStopDataFrameTransformAction.java

@@ -88,8 +120,9 @@ protected void doExecute(Task task, StopDataFrameTransformAction.Request request
                new PageParams(0, 10_000),
                request.isAllowNoMatch(),
                ActionListener.wrap(hitsAndIds -> {
+                    validateTaskState(state, hitsAndIds.v2(), request.isForce());


nit: taskstate -> taskstates (or is it tasksstates?)

hendrikmuhs

LGTM, some very minor comments

davidkyle

LGTM

…lastic#44231) * [ML][Data Frame] responding with appropriate status code when failing _stop * adding null checks for persistent task data * addressing PR comments

…44231) (#44277) * [ML][Data Frame] responding with appropriate status code when failing _stop * adding null checks for persistent task data * addressing PR comments

…44231) (#44276) * [ML][Data Frame] responding with appropriate status code when failing _stop * adding null checks for persistent task data * addressing PR comments

[ML][Data Frame] responding with appropriate status code when failing…

1b83d72

… _stop

benwtrent added >bug v8.0.0 :ml/Transform Transform v7.3.0 v7.4.0 labels Jul 11, 2019

benwtrent commented Jul 11, 2019

View reviewed changes

.../src/test/java/org/elasticsearch/xpack/dataframe/integration/DataFrameTaskFailedStateIT.java Outdated Show resolved Hide resolved

davidkyle reviewed Jul 11, 2019

View reviewed changes

...ain/java/org/elasticsearch/xpack/dataframe/action/TransportStopDataFrameTransformAction.java Outdated Show resolved Hide resolved

adding null checks for persistent task data

b960fd3

addressing PR comments

143e43e

Merge branch 'master' into bug/ml-df-handle-failed-stops

7741bee

hendrikmuhs reviewed Jul 12, 2019

View reviewed changes

hendrikmuhs approved these changes Jul 12, 2019

View reviewed changes

davidkyle approved these changes Jul 12, 2019

View reviewed changes

benwtrent merged commit 0b6676a into elastic:master Jul 12, 2019

benwtrent deleted the bug/ml-df-handle-failed-stops branch July 12, 2019 12:57

This was referenced Jul 12, 2019

[7.x] [ML][Data Frame] responding with 409 status code when failing _stop (#44231) #44276

Merged

[7.3] [ML][Data Frame] responding with 409 status code when failing _stop (#44231) #44277

Merged

codebrain mentioned this pull request Oct 14, 2019

7.4 meta ticket elastic/elasticsearch-net#4133

Closed

56 tasks

jakelandis removed the v8.0.0 label Jul 26, 2021

jakelandis added the v8.0.0-alpha1 label Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML][Data Frame] responding with 409 status code when failing _stop #44231

[ML][Data Frame] responding with 409 status code when failing _stop #44231

benwtrent commented Jul 11, 2019

elasticmachine commented Jul 11, 2019

davidkyle left a comment

davidkyle Jul 11, 2019

davidkyle Jul 11, 2019

benwtrent commented Jul 11, 2019

benwtrent commented Jul 11, 2019

hendrikmuhs Jul 12, 2019

droberts195 Jul 12, 2019

davidkyle Jul 12, 2019

benwtrent Jul 12, 2019

hendrikmuhs Jul 12, 2019

hendrikmuhs Jul 12, 2019

hendrikmuhs left a comment

davidkyle left a comment

[ML][Data Frame] responding with 409 status code when failing _stop #44231

[ML][Data Frame] responding with 409 status code when failing _stop #44231

Conversation

benwtrent commented Jul 11, 2019

elasticmachine commented Jul 11, 2019

davidkyle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benwtrent commented Jul 11, 2019

benwtrent commented Jul 11, 2019

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

hendrikmuhs left a comment

Choose a reason for hiding this comment

davidkyle left a comment

Choose a reason for hiding this comment