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: Add ML Get Job #32960

Merged
merged 14 commits into from
Aug 22, 2018
Merged

Conversation

benwtrent
Copy link
Member

This adds the GET ML Job endpoint to the HLRC.

Relates to #29827

@benwtrent
Copy link
Member Author

Pinging @elastic/ml-core

Since this was not done automatically

Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM

* not be equal to the actual length of the {@linkplain #results()} list if from
* & take or some cursor was used in the database query.
*/
public final class QueryPage<T extends ToXContent> implements ToXContentObject {
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 we do not need this class in the client. In the server side, it saves us from duplicating the structure count, List<T> from a number of responses. But on the client side, it doesn't seem like it's saving us a lot. I think it would be simpler to have the count and the List in the GetJobResponse directly. I've been doing the same for the get buckets API. Let me know what you think.

jobs = new QueryPage<>(RESULTS_FIELD);
}

public List<Job> getJobs() {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to jobs()

this.jobs.setCount(count);
}

public long getCount() {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to count()

this.jobs.setResults(jobs.stream().map(Job.Builder::build).collect(Collectors.toList()));
}

void setCount(long count) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need setters ones we change the response to not use a QueryPage.

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

@benwtrent benwtrent merged commit e2ea83d into elastic:master Aug 22, 2018
@benwtrent benwtrent deleted the feature/hlrc-ml-job-info branch August 22, 2018 02:02
benwtrent added a commit that referenced this pull request Aug 22, 2018
* HLRC: Adding GET ML Job info API

* HLRC: Adding GET Job ML API

* Fixing QueryPage license header

* Adding serialization tests, addressing minor issues

* Renaming querypage, changing the dependency on it

* Making things immutable

* Fixing build failure due to method rename
@benwtrent benwtrent added >enhancement and removed :ml Machine learning >feature labels Oct 25, 2018
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