HLRC: Adding ML Job stats #33183

benwtrent · 2018-08-27T18:12:27Z

Sorry for the size of this PR. There were a handful of Pojos that needed to be added before the GET jobs stats action could be completed.

I wanted to put them in separate PRs, but with how much refactoring is being done on the HLRC, coupled with my commits getting all out of order, I just opted to open it as is.

I am still putting things in the Protocol package for now. We can move all the Pojos at once when the new client ML package is ready (don't want to cause unnecessary merge conflicts).

This relates to (#29827)

HLRC: Adding pojos for job stats request

elasticmachine · 2018-08-27T18:12:29Z

Pinging @elastic/es-core-infra

elasticmachine · 2018-08-27T18:12:31Z

Pinging @elastic/ml-core

dimitris-athanasiou

This is a big one! Left some comments.

dimitris-athanasiou · 2018-08-29T15:23:45Z

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

@@ -285,4 +288,45 @@ public void getBucketsAsync(GetBucketsRequest request, RequestOptions options, A
                listener,
                Collections.emptySet());
     }
+
+    /**
+     * Gets one or more Machine Learning job statistics.


Rephrase to Gets usage information for one or more Machine Learning jobs?

dimitris-athanasiou · 2018-08-29T15:27:01Z

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

+
+
+/**
+ * Request object to get {@link org.elasticsearch.client.ml.job.stats.JobStats} objects with the matching `jobId`s


with the matching jobId's->for the matching jobId' ?

dimitris-athanasiou · 2018-08-29T15:27:29Z

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

+ */
+public class GetJobsStatsRequest extends ActionRequest implements ToXContentObject {
+
+    public static final ParseField JOB_ID = new ParseField("job_id");


reuse Job.ID instead

dimitris-athanasiou · 2018-08-29T15:28:41Z

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

+    @SuppressWarnings("unchecked")
+    public static final ConstructingObjectParser<GetJobsStatsRequest, Void> PARSER = new ConstructingObjectParser<>(
+        "get_jobs_stats_request",
+        true, a -> new GetJobsStatsRequest((List<String>) a[0]));


We shouldn't support unknown fields in the request objects, only the response. You can use the constructor that skips this argument all together.

dimitris-athanasiou · 2018-08-29T15:30:35Z

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

+ * `_all` explicitly gets all the jobs' statistics in the cluster
+ * An empty request (no `jobId`s) implicitly gets all the jobs' statistics in the cluster
+ */
+public class GetJobsStatsRequest extends ActionRequest implements ToXContentObject {


The name of this and the GetJobRequest needs to be consistent. Not sure if plural or singular is best. Potentially should also be consistent with the results (where I've been using plural). @droberts195 Any preference?

dimitris-athanasiou · 2018-08-29T16:45:03Z

client/rest-high-level/src/test/java/org/elasticsearch/client/ml/NodeAttributesTests.java

+
+    @Override
+    protected boolean supportsUnknownFields() {
+        return false;


this should be true

No, it shouldn't, because the internal map is just a map of strings and objects. Consequently, when random fields are added to the serialized object, and it is de-serialized, they are no longer equal as the individual internal attribute fields are not well defined.

I don't think that is the case. The unknown fields will only be at the first level, not under the attributes object.

Unknown fields are injected recursively into internal objects. Example from a failing test for when this is true

*****OBJECT PRE SHUFFLE + RANDOM FIELDS ******** { "id" : "IeAYBTguoP", "name" : "LOmZFvZJJN", "ephemeral_id" : "pNZZYyuHQC", "transport_address" : "oxceWLCGyt", "attributes" : { "key-1" : "EukbVGneBx", "key-0" : "CswxpsJUWa", "key-5" : "GEaErpPvvc", "key-4" : "aQotGStjzI", "key-3" : "juQFiTIlSy", "key-2" : "ynlRGrbNtc" } } ****ATRIBUTES POST SHUFFLE + RANDOM FIELDS***** { "key-1" : "EukbVGneBx", "key-0" : "CswxpsJUWa", "key-5" : "GEaErpPvvc", "key-4" : "aQotGStjzI", "key-3" : "juQFiTIlSy", "NrIvvAzJin" : 5240957203306958078, "key-2" : "ynlRGrbNtc" }

Do they?! Sorry Ben, I was really not expecting they would. Not a big fan of this. OK, you can ignore my comments about unknown fields in test. However, I wonder if we should add a manual test adding a top-level unknown field to safe-guard this as it is critical for the client forward compatibility.

@dimitris-athanasiou I was thinking for a separate PR adding a "shallow" check that does not recursively inject unknown fields. That way we can reuse that code for other other objects that have this same behavior.

@dimitris-athanasiou figured out how to only create random fields for the toplevel part of the object.

@Override protected Predicate<String> getRandomFieldsExcludeFilter() { return field -> !field.isEmpty(); }

This predicate means that for anything whose field path is not "" we should not add random fields. "" is the same as the root path.

XContentTestUtils.insertRandomFields calls XContentTestUtils.getInsertPaths and then utilized that supplied predicate to filter them.

dimitris-athanasiou · 2018-08-29T16:45:23Z

.../rest-high-level/src/test/java/org/elasticsearch/client/ml/job/stats/ForecastStatsTests.java

+
+    @Override
+    protected boolean supportsUnknownFields() {
+        return false;


this should be true

It cannot be as statusCounts is a map of strings and longs. Since the internal object is not well defined, I am not 100% sure if we should make the entries well defined and ignore stuff on the server side or not.

dimitris-athanasiou · 2018-08-29T16:46:18Z

.../rest-high-level/src/test/java/org/elasticsearch/client/ml/job/stats/ForecastStatsTests.java

+    }
+
+    private static SimpleStats createSimpleStats() {
+        return new SimpleStatsTests().createTestInstance();


I'd rather factoring out the creation here and then call that from createTestInstance`

I don't really understand your comment, but regardless, does not seem like that big of an issue.

I meant that it doesn't make sense to instantiate the tests class here. We could extract the body of createTestInstance into a method createRandom and then call the latter from the former.

dimitris-athanasiou · 2018-08-29T16:46:34Z

client/rest-high-level/src/test/java/org/elasticsearch/client/ml/job/stats/JobStatsTests.java

+
+    @Override
+    protected boolean supportsUnknownFields() {
+        return false;


this should be true

Similar reason to all the other reasons about why unknown fields is false here.

dimitris-athanasiou · 2018-08-29T16:48:21Z

docs/java-rest/high-level/ml/get-job-stats.asciidoc

+include-tagged::{doc-tests}/MlClientDocumentationIT.java[x-pack-ml-get-job-stats-listener]
+--------------------------------------------------
+<1> `onResponse` is called back when the action is completed successfully
+<2> `onFailure` is called back when some unexpected error occurs


we're missing a description of the response

What sort of description? in the execution section there is an explanation of what a JobsStatsResponse object looks like and contains.

Do you want a description of the JobStats object itself? That is a pretty big object that is probably best served through JavaDocs.

I've looked in some other asciidocs and they usually have a section with the response object where they have the description that you've added in the execution (not the JobStats object). See for example get-buckets.asciidoc where I copied that.

dimitris-athanasiou · 2018-08-31T14:21:36Z

client/rest-high-level/src/main/java/org/elasticsearch/client/ml/job/stats/ForecastStats.java

-    private long total;
-    private long forecastedJobs;
+    private final long total;
+    private final long forecastedJobs;
    private SimpleStats memoryStats;


The rest could be final as well, no?

@dimitris-athanasiou No, because they are optionally set in the constructor if the total is > 0

This just means we need to explicitly set them to null if the condition is not true. Just informational, it's no big deal whether we do it or not.

dimitris-athanasiou

LGTM

benwtrent · 2018-08-31T18:01:31Z

Jenkins retest this please

* HLRC: Adding pojos for get job stats HLRC: Adding pojos for job stats request * HLRC: Adding job stats pojos * HLRC: ML job stats * Minor syntax changes and adding license headers * minor comment change * Moving to client package, minor changes * Addressing PR comments * removing bad sleep * addressing minor comment around test methods * adding toplevel random fields for tests * addressing minor review comments

* master: HLRC: ML Flush job (#33187) HLRC: Adding ML Job stats (#33183) LLREST: Drop deprecated methods (#33223) Mute testSyncerOnClosingShard [DOCS] Moves machine learning APIs to docs folder (#31118)

* 6.x: HLRC: ML Flush job (#33187) Switch more LLREST usage to new style Requests (#33171) HLRC: Adding ML Job stats (#33183) HLREST: add reindex API (#32679) Mute testSyncerOnClosingShard [DOCS] Moves machine learning APIs to docs folder (#31118)

* master: (64 commits) HLREST: add update by query API (elastic#32760) TEST: Increase timeout testFollowIndexAndCloseNode (elastic#33333) HLRC: ML Flush job (elastic#33187) HLRC: Adding ML Job stats (elastic#33183) LLREST: Drop deprecated methods (elastic#33223) Mute testSyncerOnClosingShard [DOCS] Moves machine learning APIs to docs folder (elastic#31118) Mute test watcher usage stats output [Rollup] Fix FullClusterRestart test Adjust soft-deletes version after backport into 6.5 completely drop `index.shard.check_on_startup: fix` for 7.0 (elastic#33194) Fix AwaitsFix issue number Mute SmokeTestWatcherWithSecurityIT testsi drop `index.shard.check_on_startup: fix` (elastic#32279) tracked at [DOCS] Moves ml folder from x-pack/docs to docs (elastic#33248) [DOCS] Move rollup APIs to docs (elastic#31450) [DOCS] Rename X-Pack Commands section (elastic#33005) TEST: Disable soft-deletes in ParentChildTestCase Fixes SecurityIntegTestCase so it always adds at least one alias (elastic#33296) ...

* master: (283 commits) HLREST: add update by query API (elastic#32760) TEST: Increase timeout testFollowIndexAndCloseNode (elastic#33333) HLRC: ML Flush job (elastic#33187) HLRC: Adding ML Job stats (elastic#33183) LLREST: Drop deprecated methods (elastic#33223) Mute testSyncerOnClosingShard [DOCS] Moves machine learning APIs to docs folder (elastic#31118) Mute test watcher usage stats output [Rollup] Fix FullClusterRestart test Adjust soft-deletes version after backport into 6.5 completely drop `index.shard.check_on_startup: fix` for 7.0 (elastic#33194) Fix AwaitsFix issue number Mute SmokeTestWatcherWithSecurityIT testsi drop `index.shard.check_on_startup: fix` (elastic#32279) tracked at [DOCS] Moves ml folder from x-pack/docs to docs (elastic#33248) [DOCS] Move rollup APIs to docs (elastic#31450) [DOCS] Rename X-Pack Commands section (elastic#33005) TEST: Disable soft-deletes in ParentChildTestCase Fixes SecurityIntegTestCase so it always adds at least one alias (elastic#33296) ...

* master: (197 commits) Prevent NPE parsing the stop datafeed request. (elastic#33347) HLRC: Add ML get overall buckets API (elastic#33297) Core: Fix epoch millis java time formatter (elastic#33302) [Docs] Improve tuning for speed advice (elastic#33315) [Rollup] Fix Caps Comparator to handle calendar/fixed time (elastic#33336) [CI] Mute IndexShardTests#testIndexCheckOnStartup fails elastic#33345 [CI] Mute LuceneChangesSnapshotTests#testUpdateAndReadChangesConcurrently Security for _field_names field should not override field statistics (elastic#33261) Add early termination support to BucketCollector (elastic#33279) Fix extractjar task ci (elastic#33272) Mute testFollowIndexAndCloseNode Logging: Drop Settings from some logging ctors (elastic#33332) HLREST: add update by query API (elastic#32760) TEST: Increase timeout testFollowIndexAndCloseNode (elastic#33333) HLRC: ML Flush job (elastic#33187) HLRC: Adding ML Job stats (elastic#33183) LLREST: Drop deprecated methods (elastic#33223) Mute testSyncerOnClosingShard [DOCS] Moves machine learning APIs to docs folder (elastic#31118) Mute test watcher usage stats output ...

benwtrent added 8 commits August 21, 2018 16:00

HLRC: Adding pojos for get job stats

e2d0daa

HLRC: Adding pojos for job stats request

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

cefe9d3

HLRC: Adding job stats pojos

8a7feb4

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

f55e01e

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

cc602fe

HLRC: ML job stats

de52ab2

Merge branch 'master' into feature/ml-hlrc-job-stats-2

54d56a7

Minor syntax changes and adding license headers

c8d3f5a

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

elasticmachine mentioned this pull request Aug 27, 2018

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

Closed

98 tasks

benwtrent added 4 commits August 27, 2018 15:35

minor comment change

da0c1bd

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

5254cc1

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

55f543d

Moving to client package, minor changes

79dd816

dimitris-athanasiou reviewed Aug 29, 2018

View reviewed changes

benwtrent added 5 commits August 29, 2018 14:06

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

9f2bc9e

Addressing PR comments

a6512a4

removing bad sleep

2c9e231

addressing minor comment around test methods

b761884

adding toplevel random fields for tests

a45fbec

dimitris-athanasiou reviewed Aug 31, 2018

View reviewed changes

addressing minor review comments

fdf2aa0

dimitris-athanasiou approved these changes Aug 31, 2018

View reviewed changes

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

401223b

benwtrent merged commit 19b14fa into elastic:master Sep 1, 2018

benwtrent deleted the feature/ml-hlrc-job-stats branch September 1, 2018 18:32

benwtrent removed the :ml Machine learning label 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: Adding ML Job stats #33183

HLRC: Adding ML Job stats #33183

benwtrent commented Aug 27, 2018

elasticmachine commented Aug 27, 2018

elasticmachine commented Aug 27, 2018

dimitris-athanasiou left a comment

dimitris-athanasiou Aug 29, 2018

dimitris-athanasiou Aug 29, 2018

dimitris-athanasiou Aug 29, 2018

dimitris-athanasiou Aug 29, 2018

dimitris-athanasiou Aug 29, 2018

dimitris-athanasiou Aug 29, 2018

benwtrent Aug 29, 2018

dimitris-athanasiou Aug 29, 2018

benwtrent Aug 30, 2018

dimitris-athanasiou Aug 30, 2018

benwtrent Aug 30, 2018

benwtrent Aug 30, 2018

dimitris-athanasiou Aug 29, 2018

benwtrent Aug 29, 2018

dimitris-athanasiou Aug 29, 2018

benwtrent Aug 29, 2018

dimitris-athanasiou Aug 29, 2018

dimitris-athanasiou Aug 29, 2018

benwtrent Aug 29, 2018

dimitris-athanasiou Aug 29, 2018

benwtrent Aug 29, 2018

dimitris-athanasiou Aug 31, 2018

dimitris-athanasiou Aug 31, 2018

benwtrent Aug 31, 2018

dimitris-athanasiou Aug 31, 2018

dimitris-athanasiou left a comment •

edited

Loading

benwtrent commented Aug 31, 2018



		/**
		* Request object to get {@link org.elasticsearch.client.ml.job.stats.JobStats} objects with the matching `jobId`s

HLRC: Adding ML Job stats #33183

HLRC: Adding ML Job stats #33183

Conversation

benwtrent commented Aug 27, 2018

elasticmachine commented Aug 27, 2018

elasticmachine commented Aug 27, 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

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

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 • edited Loading

Choose a reason for hiding this comment

benwtrent commented Aug 31, 2018

dimitris-athanasiou left a comment •

edited

Loading