Clear Job#finished_time when it is opened (#32605) #32755

benwtrent · 2018-08-09T18:50:16Z

When a job is opened, a new action is taken now where the cluster state is updated, such that the job that was opened has its finished_time set to null.

Verified this works locally and through new integration test.

dimitris-athanasiou

Looks good. Left a few comments.

dimitris-athanasiou · 2018-08-10T08:37:04Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java

+            // Step 6. Clear job finished time once the job is started and respond
+            ActionListener<OpenJobAction.Response> clearJobFinishTime = ActionListener.wrap(
+                response -> {
+                    if(response.isAcknowledged()) {


nit: space after if

dimitris-athanasiou · 2018-08-10T08:41:15Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java

@@ -574,6 +592,35 @@ public void onTimeout(TimeValue timeout) {
        });
    }

+    private void clearJobFinishedTime(String jobId, ActionListener<OpenJobAction.Response> listener) {
+        clusterService.submitStateUpdateTask("clearing_job_finish_time [" + jobId + "]", new ClusterStateUpdateTask() {


nit: it seems that in most places we use hyphens to name the cluster state update tasks. It would be more consistent to rename this to clear-finished-time-for-job-{job_id}.

dimitris-athanasiou · 2018-08-10T08:43:37Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java

+        clusterService.submitStateUpdateTask("clearing_job_finish_time [" + jobId + "]", new ClusterStateUpdateTask() {
+            @Override
+            public ClusterState execute(ClusterState currentState) {
+                XPackPlugin.checkReadyForXPackCustomMetadata(currentState);


This check is unnecessary as if a job is being opened we are certain there is ML Metadata already installed.

dimitris-athanasiou · 2018-08-10T08:48:42Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java

+
+            @Override
+            public void onFailure(String source, Exception e) {
+                listener.onFailure(e);


I am tempted to handle failure differently here. Even if the cluster state update failed, the job has successfully opened. It might be more misleading to return an error to the user, as they may think the job has not been opened. I wonder if logging the error should be enough. Any thoughts @droberts195 ?

I too considered just logging the issue and still letting the end user know that it succeeded. Definitely a contentious decision that needs discussion.

I think we should just log the error. As @dimitris-athanasiou said, if it fails here the job will have a status of open. So anyone who reacted to the failure to open by trying again would be bemused to find out that they couldn't open the job the second time because it was already open.

dimitris-athanasiou · 2018-08-10T08:48:57Z

...ests/src/test/java/org/elasticsearch/xpack/ml/integration/ReopenJobResetsFinishedTimeIT.java

@@ -0,0 +1,57 @@
+package org.elasticsearch.xpack.ml.integration;


Missing license header

dimitris-athanasiou · 2018-08-10T08:49:50Z

...ests/src/test/java/org/elasticsearch/xpack/ml/integration/ReopenJobResetsFinishedTimeIT.java

+import static org.hamcrest.CoreMatchers.nullValue;
+import static org.hamcrest.Matchers.is;
+
+public class ReopenJobResetsFinishedTimeIT extends MlNativeAutodetectIntegTestCase {


Awesome! :-)

davidkyle · 2018-08-10T09:43:11Z

The build failed with checkstyle

[ant:checkstyle] [ERROR] /var/lib/jenkins/workspace/elastic+elasticsearch+pull-request/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java:58:8: Unused import - org.elasticsearch.xpack.core.ml.action.FinalizeJobExecutionAction. [UnusedImports]

If you want to run this check locally use ./gradlew precommit

colings86 · 2018-08-10T14:15:03Z

@benwtrent Can you please add labels to this PR, including version, area and change type labels

elasticmachine · 2018-08-10T14:15:16Z

Pinging @elastic/ml-core

dimitris-athanasiou · 2018-08-10T14:56:04Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java

@@ -611,7 +607,8 @@ public ClusterState execute(ClusterState currentState) {

            @Override
            public void onFailure(String source, Exception e) {
-                listener.onFailure(e);
+                logger.error(source + "Failed to clear finished_time due to [" + e.getMessage() + "]", e);


We tend to start the messages with the job_id. So, this could become:

logger.error("[" + jobId + "] Failed to clear finished_time; source [" + source + "]", e);

dimitris-athanasiou · 2018-08-10T14:57:01Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java

@@ -593,10 +590,9 @@ public void onTimeout(TimeValue timeout) {
    }

    private void clearJobFinishedTime(String jobId, ActionListener<OpenJobAction.Response> listener) {
-        clusterService.submitStateUpdateTask("clearing_job_finish_time [" + jobId + "]", new ClusterStateUpdateTask() {
+        clusterService.submitStateUpdateTask("clearing-job-finish-time [" + jobId + "]", new ClusterStateUpdateTask() {


sorry for insisting on that but so that task names are consistent (should make it easier to search through them) could you change to "clearing-finish-time-for-job-" + jobId?

dimitris-athanasiou

LGTM

* Clear Job#finished_time when it is opened (#32605) * not returning failure when Job#finished_time is not reset * Changing error log string and source string

…listeners * elastic/master: (58 commits) [ML] Partition-wise maximum scores (elastic#32748) [DOCS] XContentBuilder#bytes method removed, using BytesReference.bytes(docBuilder) (elastic#32771) HLRC: migration get assistance API (elastic#32744) Add a task to run forbiddenapis using cli (elastic#32076) [Kerberos] Add debug log statement for exceptions (elastic#32663) Make x-pack core pull transport-nio (elastic#32757) Painless: Clean Up Whitelist Names (elastic#32791) Cat apis: Fix index creation time to use strict date format (elastic#32510) Clear Job#finished_time when it is opened (elastic#32605) (elastic#32755) Test: Only sniff host metadata for node_selectors (elastic#32750) Update scripted metric docs to use `state` variable (elastic#32695) Painless: Clean up PainlessCast (elastic#32754) [TEST] Certificate NONE not allowed in FIPS JVM (elastic#32753) [ML] Refactor ProcessCtrl into Autodetect and Normalizer builders (elastic#32720) Access build tools resources (elastic#32201) Tests: Disable rolling upgrade tests with system key on fips JVM (elastic#32775) HLRC: Ban LoggingDeprecationHandler (elastic#32756) Fix test reproducability in AbstractBuilderTestCase setup (elastic#32403) Only require java<version>_home env var if needed Tests: Muted ScriptDocValuesDatesTests.testJodaTimeBwc ...

Clear Job#finished_time when it is opened (elastic#32605)

08218af

dimitris-athanasiou reviewed Aug 10, 2018

View reviewed changes

colings86 added the :ml Machine learning label Aug 10, 2018

benwtrent added the >bug label Aug 10, 2018

dimitris-athanasiou added review v7.0.0 v6.5.0 labels Aug 10, 2018

not returning failure when Job#finished_time is not reset

75913a9

dimitris-athanasiou reviewed Aug 10, 2018

View reviewed changes

Changing error log string and source string

d1bbd55

dimitris-athanasiou approved these changes Aug 10, 2018

View reviewed changes

benwtrent merged commit b08416b into elastic:master Aug 10, 2018

benwtrent mentioned this pull request Aug 10, 2018

[ML] 'finished_time' still reported after job has been reopened #32605

Closed

benwtrent deleted the bug/clear-finished_time-on-job-open branch November 26, 2018 16:40

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear Job#finished_time when it is opened (#32605) #32755

Clear Job#finished_time when it is opened (#32605) #32755

benwtrent commented Aug 9, 2018

dimitris-athanasiou left a comment

dimitris-athanasiou Aug 10, 2018

dimitris-athanasiou Aug 10, 2018

dimitris-athanasiou Aug 10, 2018

dimitris-athanasiou Aug 10, 2018

benwtrent Aug 10, 2018

droberts195 Aug 10, 2018

dimitris-athanasiou Aug 10, 2018

dimitris-athanasiou Aug 10, 2018

davidkyle commented Aug 10, 2018

colings86 commented Aug 10, 2018

elasticmachine commented Aug 10, 2018

dimitris-athanasiou Aug 10, 2018

dimitris-athanasiou Aug 10, 2018

dimitris-athanasiou left a comment

		@@ -0,0 +1,57 @@
		package org.elasticsearch.xpack.ml.integration;

Clear Job#finished_time when it is opened (#32605) #32755

Clear Job#finished_time when it is opened (#32605) #32755

Conversation

benwtrent commented Aug 9, 2018

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

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 Aug 10, 2018

colings86 commented Aug 10, 2018

elasticmachine commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitris-athanasiou left a comment

Choose a reason for hiding this comment