Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ML] Return assigned node in start/open job/datafeed response #55473

Merged
merged 8 commits into from
Apr 22, 2020
Merged

[ML] Return assigned node in start/open job/datafeed response #55473

merged 8 commits into from
Apr 22, 2020

Conversation

droberts195
Copy link
Contributor

@droberts195 droberts195 commented Apr 20, 2020 •
edited
Loading

Adds a "node" field to the response from the following endpoints:

  1. Open anomaly detection job
  2. Start datafeed
  3. Start data frame analytics job

If the job or datafeed is assigned to a node immediately then
this field will return the ID of that node.

In the case where a job or datafeed is opened or started lazily
the node field will contain an empty string. Clients that want
to test whether a job or datafeed was opened or started lazily
can therefore check for this.

Fixes #54067

@droberts195
[ML] Return assigned node in start/open job/datafeed response
80400f9 
Adds a "node" field to the response from the following endpoints:

1. Open anomaly detection job
2. Start datafeed
3. Start data frame analytics job

If the job or datafeed is assigned to a node immediately then
this field will return the ID of that node.

In the case where a job or datafeed is opened or started lazily
the node field will contain an empty string.  Clients that want
to test whether a job or datafeed was opened or started lazily
can therefore check for this.

Fixes elastic#54067
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

@benwtrent benwtrent self-requested a review April 20, 2020 16:55
benwtrent
benwtrent approved these changes Apr 20, 2020
View reviewed changes
...core/src/test/java/org/elasticsearch/xpack/core/ml/action/NodeAcknowledgedResponseTests.java Outdated
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.test.AbstractWireSerializingTestCase;

public class NodeAcknowledgedResponseTests extends AbstractWireSerializingTestCase<NodeAcknowledgedResponse> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: might be cool to use AbstractBWCWireSerializationTestCase instead.

...ugin/ml/src/main/java/org/elasticsearch/xpack/ml/rest/datafeeds/RestStartDatafeedAction.java Outdated Show resolved Hide resolved
przemekwitek
przemekwitek approved these changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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

static {
PARSER.declareBoolean(ConstructingObjectParser.constructorArg(), OPENED);
PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), NODE);
}

private boolean opened;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the fields in this class can be final.

client/rest-high-level/src/main/java/org/elasticsearch/client/ml/OpenJobResponse.java Outdated
@@ -71,18 +88,22 @@ public boolean equals(Object other) {
}

OpenJobResponse that = (OpenJobResponse) other;
return isOpened() == that.isOpened();
return isOpened() == that.isOpened()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It feels inconsistent that we compare one field using the accessor and the other one directly. Could you stick to one of those styles in equals and hashCode?

Or maybe it would be possible to derive from AcknowledgedResponse (it would have to become configurable wrt acknowledged field name though...).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would have to become configurable wrt acknowledged field name though

I don't think this would be a good idea. We made a mistake really in not using acknowledged as the field in this class back in 2016. We stuck to the Prelert heritage instead of making things consistent with the Elastic way. Unfortunately now making it consistent would be breaking change, and it doesn't seem like it's a big enough problem to warrant the break. But making it easier for others to be inconsistent in the future doesn't seem like a good idea.

client/rest-high-level/src/main/java/org/elasticsearch/client/ml/StartDatafeedResponse.java Outdated
@@ -74,18 +90,22 @@ public boolean equals(Object other) {
}

StartDatafeedResponse that = (StartDatafeedResponse) other;
return isStarted() == that.isStarted();
return isStarted() == that.isStarted()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar remark here.

@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/default-distro

@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/default-distro

@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/1

@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/packaging-sample-unix-docker

@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/1

@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/packaging-sample-unix-docker

@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/1

@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/packaging-sample-matrix-windows

@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/packaging-sample-unix-docker

@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/2

@droberts195 droberts195 merged commit 8906e76 into elastic:master Apr 22, 2020
@droberts195 droberts195 deleted the add_node_to_start_open_response branch April 22, 2020 07:45
droberts195 added a commit to droberts195/elasticsearch that referenced this pull request Apr 22, 2020
@droberts195
[ML] Return assigned node in start/open job/datafeed response
e05d54c 
Adds a "node" field to the response from the following endpoints:

1. Open anomaly detection job
2. Start datafeed
3. Start data frame analytics job

If the job or datafeed is assigned to a node immediately then
this field will return the ID of that node.

In the case where a job or datafeed is opened or started lazily
the node field will contain an empty string.  Clients that want
to test whether a job or datafeed was opened or started lazily
can therefore check for this.

Backport of elastic#55473
@droberts195 droberts195 mentioned this pull request Apr 22, 2020
Merged
droberts195 added a commit to droberts195/elasticsearch that referenced this pull request Apr 22, 2020
@droberts195
[ML] Adjust BWC for node in start/open responses following backport
b747adf 
Once elastic#55570 is merged the master branch BWC code is no longer needed.

Relates elastic#55473
@droberts195 droberts195 mentioned this pull request Apr 22, 2020
Merged
droberts195 added a commit that referenced this pull request Apr 22, 2020
@droberts195
[ML] Return assigned node in start/open job/datafeed response (#55570)
da5aeb8 
Adds a "node" field to the response from the following endpoints:

1. Open anomaly detection job
2. Start datafeed
3. Start data frame analytics job

If the job or datafeed is assigned to a node immediately then
this field will return the ID of that node.

In the case where a job or datafeed is opened or started lazily
the node field will contain an empty string.  Clients that want
to test whether a job or datafeed was opened or started lazily
can therefore check for this.

Backport of #55473
droberts195 added a commit that referenced this pull request Apr 22, 2020
@droberts195
[ML] Adjust BWC for node in start/open responses following backport (#…
129a01f 
…55573)

Once #55570 is merged the master branch BWC code is no longer needed.

Relates #55473
@russcam russcam mentioned this pull request May 29, 2020
Closed
17 tasks
@russcam russcam mentioned this pull request Jun 23, 2020
Closed
4 tasks
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 24, 2020
@russcam
Add assigned node to OpenJob/StartDatafeed response
02e0fe5 
Relates: elastic/elasticsearch#55473, #4803
@russcam russcam mentioned this pull request Jun 24, 2020
Merged
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 29, 2020
@russcam
Add assigned node to OpenJob/StartDatafeed response (#4815)
3ed723d 
Relates: elastic/elasticsearch#55473, #4803
github-actions bot pushed a commit to elastic/elasticsearch-net that referenced this pull request Jun 29, 2020
@russcam @github-actions
Add assigned node to OpenJob/StartDatafeed response (#4815)
ab80edf 
Relates: elastic/elasticsearch#55473, #4803
github-actions bot pushed a commit to elastic/elasticsearch-net that referenced this pull request Jun 29, 2020
@russcam @github-actions
Add assigned node to OpenJob/StartDatafeed response (#4815)
ddb9480 
Relates: elastic/elasticsearch#55473, #4803
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 30, 2020
@github-actions @russcam
Add assigned node to OpenJob/StartDatafeed response (#4815) (#4827)
41959c3 
Relates: elastic/elasticsearch#55473, #4803

Co-authored-by: Russ Cam <[email protected]>
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 30, 2020
@github-actions @russcam
Add assigned node to OpenJob/StartDatafeed response (#4815) (#4828)
f2d0139 
Relates: elastic/elasticsearch#55473, #4803

Co-authored-by: Russ Cam <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcawl lcawl lcawl left review comments

@przemekwitek przemekwitek przemekwitek approved these changes

@benwtrent benwtrent benwtrent approved these changes

Assignees
No one assigned
Labels
>enhancement :ml Machine learning v7.8.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ML] Make it easier to see when jobs/datafeeds are lazily started
6 participants
@droberts195 @elasticmachine @benwtrent @przemekwitek @lcawl @jakelandis