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.

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

HLRC: Adding Update datafeed API #34882

Merged
merged 5 commits into from
Oct 26, 2018

Conversation

benwtrent
Copy link
Member

HLRC: ML Adds the Update Datafeed API

Relates to #29827

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@benwtrent
Copy link
Member Author

Pinging @elastic/ml-core

Copy link
Contributor

@dimitris-athanasiou dimitris-athanasiou left a comment

Choose a reason for hiding this comment

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

Looks good. Left a few minor comments.

@@ -209,6 +210,19 @@ static Request putDatafeed(PutDatafeedRequest putDatafeedRequest) throws IOExcep
return request;
}

static Request updateDatafeed(UpdateDatafeedRequest updateDatafeedRequest) throws IOException {
String endpoint = new EndpointBuilder()
.addPathPart("").addPathPartAsIs("_xpack")
Copy link
Contributor

Choose a reason for hiding this comment

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

The first empty part seems redundant

Copy link
Member Author

Choose a reason for hiding this comment

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

definitely :D copy paste error

@@ -494,6 +495,46 @@ public void putDatafeedAsync(PutDatafeedRequest request, RequestOptions options,
Collections.emptySet());
}

/**
* Updated a Machine Learning Datafeed
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: Updated -> Updates

import java.util.Objects;

/**
* Updates a {@link org.elasticsearch.client.ml.datafeed.DatafeedConfig} with the passed {@link DatafeedUpdate}
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment implies this class is the action. Reword to suit it is a request?

machineLearningClient::updateDatafeed,
machineLearningClient::updateDatafeedAsync);

DatafeedConfig updatedDatafeed= response.getResponse();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing space before equals

<1> The updated configuration of the {ml} datafeed

[id="{upid}-{api}-config"]
==== Updated Datafeed Configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

This title/section might be unnecessary. Check update-job documentation. We could have a similar structure here unless you dislike that one.

Copy link
Contributor

@dimitris-athanasiou dimitris-athanasiou left a comment

Choose a reason for hiding this comment

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

LGTM apart from a little typo

import java.util.Objects;

/**
* Requests an updates to a {@link org.elasticsearch.client.ml.datafeed.DatafeedConfig} with the passed {@link DatafeedUpdate}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: updates -> update

@benwtrent benwtrent merged commit 052dfa5 into elastic:master Oct 26, 2018
@benwtrent benwtrent deleted the feature/hlrc-ml-update-datafeed branch October 26, 2018 21:44
benwtrent added a commit that referenced this pull request Oct 26, 2018
* HLRC: Adding Update datafeed API

* Addressing unused import

* Adjusting docs and fixing minor comments

* fixing comment
kcm pushed a commit that referenced this pull request Oct 30, 2018
* HLRC: Adding Update datafeed API

* Addressing unused import

* Adjusting docs and fixing minor comments

* fixing comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants