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: ML Add preview datafeed api #34284

Merged

Conversation

benwtrent
Copy link
Member

This adds the ML preview datafeed API to the HLRC

Relates to #29827

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

I left a couple of nits on the docs and also something worth investigating for whether we can reduce the duplication in the docs. If it is possible to reduce the duplication and you'd rather leave that for a followup PR that applies to change to all our docs then that's fine.

--------------------------------------------------
include-tagged::{doc-tests}/MlClientDocumentationIT.java[x-pack-ml-preview-datafeed-execute]
--------------------------------------------------
<1> The raw BytesReference of the data preview
Copy link
Contributor

Choose a reason for hiding this comment

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

BytesReference would look better in backticks: BytesReference

include-tagged::{doc-tests}/MlClientDocumentationIT.java[x-pack-ml-preview-datafeed-execute]
--------------------------------------------------
<1> The raw BytesReference of the data preview
<2> A List of Map<String,Object> that represents the previewed data
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly List and Map<String, Object> would look better in backticks.

Also, I think we usually put a space after the comma in Map<String, Object>. It's not so important for private code but it's nice to make the public docs completely consistent.

include-tagged::{doc-tests}/MlClientDocumentationIT.java[x-pack-ml-preview-datafeed-listener]
--------------------------------------------------
<1> `onResponse` is called back when the action is completed successfully
<2> `onFailure` is called back when some unexpected error occurs
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that we can reuse the stuff that was added in #34125 to cut out the duplication in this async section?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not know that was a thing! will update this PR and my other HLRC one pronto

Copy link
Contributor

@droberts195 droberts195 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 2dd058d into elastic:master Oct 4, 2018
@benwtrent benwtrent deleted the feature/hlrc-ml-preview-datafeed branch October 4, 2018 18:28
benwtrent added a commit that referenced this pull request Oct 4, 2018
* HLRC: ML Add preview datafeed api

* Changing deprecation handling for parser

* Removing some duplication in docs, will address other APIs in another PR
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Oct 4, 2018
* master: (25 commits)
  HLRC: ML Adding get datafeed stats API (elastic#34271)
  Small fixes to the HLRC watcher documentation. (elastic#34306)
  Tasks: Document that status is not semvered (elastic#34270)
  HLRC: ML Add preview datafeed api (elastic#34284)
  [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation
  Fix error in documentation for activete watch
  SCRIPTING: Terms set query expression (elastic#33856)
  Logging: Drop remaining Settings log ctor (elastic#34149)
  [ML] Remove unused last_data_time member from Job (elastic#34262)
  Docs: Allow skipping response assertions (elastic#34240)
  HLRC: Add activate watch action (elastic#33988)
  [Security] Multi Index Expression alias wildcard exclusion (elastic#34144)
  [ML] Label anomalies with  multi_bucket_impact (elastic#34233)
  Document smtp.ssl.trust configuration option (elastic#34275)
  Support PKCS#11 tokens as keystores and truststores  (elastic#34063)
  Fix sporadic failure in NestedObjectMapperTests
  [Authz] Allow update settings action for system user (elastic#34030)
  Replace version with reader cache key in IndicesRequestCache (elastic#34189)
  [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211)
  Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Oct 4, 2018
* rename-ccr-stats: (25 commits)
  HLRC: ML Adding get datafeed stats API (elastic#34271)
  Small fixes to the HLRC watcher documentation. (elastic#34306)
  Tasks: Document that status is not semvered (elastic#34270)
  HLRC: ML Add preview datafeed api (elastic#34284)
  [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation
  Fix error in documentation for activete watch
  SCRIPTING: Terms set query expression (elastic#33856)
  Logging: Drop remaining Settings log ctor (elastic#34149)
  [ML] Remove unused last_data_time member from Job (elastic#34262)
  Docs: Allow skipping response assertions (elastic#34240)
  HLRC: Add activate watch action (elastic#33988)
  [Security] Multi Index Expression alias wildcard exclusion (elastic#34144)
  [ML] Label anomalies with  multi_bucket_impact (elastic#34233)
  Document smtp.ssl.trust configuration option (elastic#34275)
  Support PKCS#11 tokens as keystores and truststores  (elastic#34063)
  Fix sporadic failure in NestedObjectMapperTests
  [Authz] Allow update settings action for system user (elastic#34030)
  Replace version with reader cache key in IndicesRequestCache (elastic#34189)
  [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211)
  Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247)
  ...
@benwtrent benwtrent removed the :ml Machine learning label Oct 25, 2018
kcm pushed a commit that referenced this pull request Oct 30, 2018
* HLRC: ML Add preview datafeed api

* Changing deprecation handling for parser

* Removing some duplication in docs, will address other APIs in another PR
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.

None yet

4 participants