Skip to content

HLRC: Add ML Get Job#32960

Merged
benwtrent merged 14 commits intoelastic:masterfrom
benwtrent:feature/hlrc-ml-job-info
Aug 22, 2018
Merged

HLRC: Add ML Get Job#32960
benwtrent merged 14 commits intoelastic:masterfrom
benwtrent:feature/hlrc-ml-job-info

Conversation

@benwtrent
Copy link
Copy Markdown
Member

This adds the GET ML Job endpoint to the HLRC.

Relates to #29827

@benwtrent
Copy link
Copy Markdown
Member Author

Pinging @elastic/ml-core

Since this was not done automatically

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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