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 categories API #33465

Merged
merged 6 commits into from
Sep 11, 2018

Conversation

edsavage
Copy link
Contributor

@edsavage edsavage commented Sep 6, 2018

Relates #29827

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

* ML GET categories documentation</a>
*
* @param request The request
* @param options Additional request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
Copy link
Member

Choose a reason for hiding this comment

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

I know some methods add the @throws and others don't. I think we should still add the @throws entry even if it is a checked exception.

I have not been consistent about this myself either :(.

Copy link
Member

Choose a reason for hiding this comment

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

@dimitris-athanasiou may have a different opinion around this ^, So I will defer to him.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right @benwtrent, we've been dropping the @throws clause in some of the methods in the client. We'll need to revisit and add them. I'll make a note to do that.


public static final ParseField CATEGORY_ID = new ParseField("category_id");

public static final ObjectParser<GetCategoriesRequest, Void> PARSER =
Copy link
Member

Choose a reason for hiding this comment

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

This should be ConstructingObjectParser

PARSER.declareObject(GetCategoriesRequest::setPageParams, PageParams.PARSER, PageParams.PAGE);
}

private String jobId;
Copy link
Member

Choose a reason for hiding this comment

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

This should be final

private Long categoryId;
private PageParams pageParams;

private GetCategoriesRequest() {}
Copy link
Member

Choose a reason for hiding this comment

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

We should not have this ctr, and just use the ConstructingObjectParser to pass the jobId as it is required.

@@ -90,6 +92,9 @@ public void createJobAndIndexResults() throws IOException {
addBucketIndexRequest(time, true, bulkRequest);
addRecordIndexRequest(time, true, bulkRequest);

// index some category results
addCategoriesIndexRequests(bulkRequest);
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 could perform this just in the get categories test. However, I suppose it doesn't really add to the execution time. Note that this setup is run once per test. Might be fun trying to see if there's a noticeable difference. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! That call is only indexing 3 category results so I suspect the impact on execution time is low but I'll make the change regardless. Anything we can do to speed up the tests has to be a good thing...

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

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.

LGTM2

@edsavage edsavage merged commit 2f3b542 into elastic:master Sep 11, 2018
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 11, 2018
* master:
  Preserve cluster settings on full restart tests (elastic#33590)
  Use IndexWriter.getFlushingBytes() rather than tracking it ourselves (elastic#33582)
  Fix upgrading of list settings (elastic#33589)
  Add read-only Engine (elastic#33563)
  HLRC: Add ML get categories API (elastic#33465)
  SQL: Adds MONTHNAME, DAYNAME and QUARTER functions (elastic#33411)
  Add predicate_token_filter (elastic#33431)
  Fix Replace function. Adds more tests to all string functions. (elastic#33478)
  [ML] Rename input_fields to column_names in file structure (elastic#33568)
jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Sep 11, 2018
* master: (91 commits)
  Preserve cluster settings on full restart tests (elastic#33590)
  Use IndexWriter.getFlushingBytes() rather than tracking it ourselves (elastic#33582)
  Fix upgrading of list settings (elastic#33589)
  Add read-only Engine (elastic#33563)
  HLRC: Add ML get categories API (elastic#33465)
  SQL: Adds MONTHNAME, DAYNAME and QUARTER functions (elastic#33411)
  Add predicate_token_filter (elastic#33431)
  Fix Replace function. Adds more tests to all string functions. (elastic#33478)
  [ML] Rename input_fields to column_names in file structure (elastic#33568)
  Add full cluster restart base class (elastic#33577)
  Validate list values for settings (elastic#33503)
  Copy and validatie soft-deletes setting on resize (elastic#33517)
  Test: Fix package name
  SQL: Fix result column names for arithmetic functions (elastic#33500)
  Upgrade to latest Lucene snapshot (elastic#33505)
  Enable not wiping cluster settings after REST test (elastic#33575)
  MINOR: Remove Dead Code in SearchScript (elastic#33569)
  [Test] Remove duplicate method in TestShardRouting (elastic#32815)
  mute test on windows
  Update beats template to include apm-server metrics (elastic#33286)
  ...
edsavage added a commit that referenced this pull request Sep 11, 2018
HLRC: Adding the ML 'get categories' API
@benwtrent benwtrent removed the :ml Machine learning label Oct 25, 2018
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
@edsavage edsavage deleted the add-ml-get-categories-api-to-hlrc branch April 12, 2019 09:22
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.

5 participants