HLRC: ML Close Job #32943

benwtrent · 2018-08-17T12:54:34Z

Adds the ML Close Job API to the HLRC.

Relates to #29827

HLRC: Adding ML Close Job API

elasticmachine · 2018-08-17T12:54:36Z

Pinging @elastic/ml-core

elasticmachine · 2018-08-17T17:13:53Z

Pinging @elastic/es-core-infra

dimitris-athanasiou · 2018-08-20T12:10:31Z

x-pack/protocol/src/main/java/org/elasticsearch/protocol/xpack/ml/CloseJobRequest.java

+        return new CloseJobRequest(ALL_JOBS);
+    }
+
+    public CloseJobRequest(String jobId) {


Shall we add a constructor that takes a List<String> and make jobIds final? As the job IDs is the required part of the request, I think replacing the setter with a constructor guides the user better towards making a valid request.

Actually, even better, we can have a single constructor that takes (String... jobIds).

We can do that, I just tested and doing a close with an empty string is a valid request, it just does not do anything. This could indeed cause confusion and possibly latent bugs in a consumer's code base.

dimitris-athanasiou

Left a few comments. I'll revisit the documentation after any changes that could result from the review.

dimitris-athanasiou · 2018-08-20T12:17:05Z

client/rest-high-level/src/main/java/org/elasticsearch/client/MLRequestConverters.java

+        Request request = new Request(HttpPost.METHOD_NAME, endpoint);
+
+        RequestConverters.Params params = new RequestConverters.Params(request);
+        params.putParam("force", Boolean.toString(closeJobRequest.isForce()));


The need to pass these as params instead of serializing the request revealed a bug: we do not support sending these params as the request body for the close job API. It'd be nice to fix that. Then this converter can follow suit with all the others.

👍 Cool, I will create a Github issue for that. It did seem kind of strange as this was the odd API endpoint out :).

dimitris-athanasiou · 2018-08-20T12:18:25Z

client/rest-high-level/src/main/java/org/elasticsearch/client/MachineLearningClient.java

@@ -166,4 +168,42 @@ public void openJobAsync(OpenJobRequest request, RequestOptions options, ActionL
            listener,
            Collections.emptySet());
    }
+
+    /**
+     * Closes Machine Learning Jobs


nit: Merge this into the next sentence? Something like Closes one or more Machine Learning jobs...

dimitris-athanasiou · 2018-08-20T12:22:28Z

client/rest-high-level/src/main/java/org/elasticsearch/client/MachineLearningClient.java

+     *
+     * A closed job cannot receive data or perform analysis operations, but you can still explore and navigate results.
+     *
+     * @param request request containing jobIds and additional optional options


I don't think we should use jobIds here. We refer to it as job_id in the open job doc. Perhaps rephrase like request containing the job_id for each job to close and additional optional options. Hm, typing optional options also felt funny. Perhaps remove optional from both?

dimitris-athanasiou · 2018-08-20T12:23:26Z

client/rest-high-level/src/main/java/org/elasticsearch/client/MachineLearningClient.java

+    }
+
+    /**
+     * Closes Machine Learning Jobs asynchronously, notifies listener on completion


Similar comments as above.

dimitris-athanasiou · 2018-08-20T12:25:32Z

x-pack/protocol/src/main/java/org/elasticsearch/protocol/xpack/ml/CloseJobRequest.java

+
+public class CloseJobRequest extends ActionRequest {
+
+    public static final String ALL_JOBS = "_all";


make this private?

dimitris-athanasiou · 2018-08-20T12:28:54Z

x-pack/protocol/src/main/java/org/elasticsearch/protocol/xpack/ml/CloseJobRequest.java

+    private List<String> jobIds = new ArrayList<>();
+    private TimeValue timeout;
+    private boolean force;
+    private boolean allowNoJobs = true;


make Boolean and only serialize when not null. Also remove setting the default. The idea here is that by doing so we inherit the defaults of the backend without having to duplicate them in the client.

dimitris-athanasiou · 2018-08-20T12:30:16Z

x-pack/protocol/src/main/java/org/elasticsearch/protocol/xpack/ml/CloseJobRequest.java

+        this.allowNoJobs = allowNoJobs;
+    }
+
+    public String getCommaDelimitedJobIdString() {


I wonder if this really belongs in the request converter. It is the request logic that requires this bit of processing so we could simply call this utility method in the request conversion.

dimitris-athanasiou · 2018-08-20T12:30:50Z

x-pack/protocol/src/main/java/org/elasticsearch/protocol/xpack/ml/CloseJobRequest.java

+        CloseJobRequest that = (CloseJobRequest) other;
+        return Objects.equals(jobIds, that.jobIds) &&
+            Objects.equals(timeout, that.timeout) &&
+            allowNoJobs == that.allowNoJobs &&


use Objects.equals for all once changed to potentially null references.

dimitris-athanasiou · 2018-08-20T12:33:51Z

x-pack/protocol/src/test/java/org/elasticsearch/protocol/xpack/ml/CloseJobRequestTests.java

+import java.util.Arrays;
+import java.util.List;
+
+public class CloseJobRequestTests extends ESTestCase {


We should be testing serialization here by extending AbstractXContentTestCase. Unfortunately, that means we need to also write a parser for the request but it's worth it.

dimitris-athanasiou · 2018-08-20T12:34:37Z

docs/java-rest/high-level/ml/close-job.asciidoc

+[[java-rest-high-x-pack-ml-close-job-request]]
+==== Close Job Request
+
+An `CloseJobRequest` object gets created with an existing non-null `jobId`.


An -> A

dimitris-athanasiou

Looks good. Just left one more comment.

dimitris-athanasiou · 2018-08-20T16:15:32Z

x-pack/protocol/src/main/java/org/elasticsearch/protocol/xpack/ml/CloseJobRequest.java

-            throw new NullPointerException("[jobId] must not be null");
-        }
-        this.jobIds = new ArrayList<>(jobIds);
+    CloseJobRequest(List<String> jobIds) {


As what we're actually storing is the list, I would make this constructor the leaf one. Then the varargs one can just call this(Arrays.asList(jobIds)).

dimitris-athanasiou

LGTM

…e-types * elastic/master: (89 commits) Fix assertion in AbstractSimpleTransportTestCase (elastic#32991) [DOC] Splits role mapping APIs into separate pages (elastic#32797) HLRC: ML Close Job (elastic#32943) Generalize remote license checker (elastic#32971) Trim translog when safe commit advanced (elastic#32967) Fix an inaccuracy in the dynamic templates documentation. (elastic#32890) Logging: Use settings when building daemon threads (elastic#32751) All Translog inner closes should happen after tragedy exception is set (elastic#32674) HLREST: AwaitsFix ML Test Pass DiscoveryNode to initiateChannel (elastic#32958) Add mzn and dz to unsupported locales (elastic#32957) Use settings from the context in BootstrapChecks (elastic#32908) Update docs for node specifications (elastic#30468) HLRC: Forbid all Elasticsearch logging infra (elastic#32784) Only configure publishing if it's applied externally (elastic#32351) Fixes libs:dissect when in eclipse Protect ScriptedMetricIT test cases against failures on 0-doc shards (elastic#32959) (elastic#32968) [Kerberos] Add documentation for Kerberos realm (elastic#32662) Watcher: Properly find next valid date in cron expressions (elastic#32734) Fix some small issues in the getting started docs (elastic#30346) ...

* HLRC: Adding ML Close Job API HLRC: Adding ML Close Job API * reconciling request converters * Adding serialization tests and addressing PR comments * Changing constructor order

benwtrent added 3 commits August 16, 2018 13:42

HLRC: Adding ML Close Job API

338e58d

HLRC: Adding ML Close Job API

Merge branch 'master' into feature/hlrc-ml-close-job

2d787d4

reconciling request converters

ea7a7bd

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

Merge branch 'master' into feature/hlrc-ml-close-job

248cd3e

benwtrent added the :Core/Java High Level REST Client label Aug 17, 2018

Merge branch 'master' into feature/hlrc-ml-close-job

cb18cbf

dimitris-athanasiou reviewed Aug 20, 2018

View reviewed changes

Adding serialization tests and addressing PR comments

9b0d4db

dimitris-athanasiou reviewed Aug 20, 2018

View reviewed changes

Changing constructor order

d6cfd1a

dimitris-athanasiou approved these changes Aug 20, 2018

View reviewed changes

benwtrent merged commit 3fbaae1 into elastic:master Aug 20, 2018

benwtrent deleted the feature/hlrc-ml-close-job branch August 20, 2018 21:06

benwtrent added a commit that referenced this pull request Aug 21, 2018

HLRC: ML Close Job (#32943)

9b36f84

* HLRC: Adding ML Close Job API HLRC: Adding ML Close Job API * reconciling request converters * Adding serialization tests and addressing PR comments * Changing constructor order

elasticmachine mentioned this pull request Aug 27, 2018

Java high-level REST client completeness for xpack APIs #29827

Closed

98 tasks

benwtrent added >enhancement and removed :ml Machine learning >feature labels Oct 25, 2018

colings86 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

HLRC: ML Close Job #32943

HLRC: ML Close Job #32943

benwtrent commented Aug 17, 2018

elasticmachine commented Aug 17, 2018

elasticmachine commented Aug 17, 2018

dimitris-athanasiou Aug 20, 2018

dimitris-athanasiou Aug 20, 2018

benwtrent Aug 20, 2018

dimitris-athanasiou left a comment

dimitris-athanasiou Aug 20, 2018

benwtrent Aug 20, 2018

dimitris-athanasiou Aug 20, 2018

dimitris-athanasiou Aug 20, 2018

dimitris-athanasiou Aug 20, 2018

dimitris-athanasiou Aug 20, 2018

dimitris-athanasiou Aug 20, 2018

dimitris-athanasiou Aug 20, 2018

dimitris-athanasiou Aug 20, 2018

dimitris-athanasiou Aug 20, 2018

dimitris-athanasiou Aug 20, 2018

dimitris-athanasiou left a comment

dimitris-athanasiou Aug 20, 2018

dimitris-athanasiou left a comment


		public class CloseJobRequest extends ActionRequest {

		public static final String ALL_JOBS = "_all";

HLRC: ML Close Job #32943

HLRC: ML Close Job #32943

Conversation

benwtrent commented Aug 17, 2018

elasticmachine commented Aug 17, 2018

elasticmachine commented Aug 17, 2018

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

dimitris-athanasiou left a comment

Choose a reason for hiding this comment