Skip to content

HLRC: ML PUT Calendar#33362

Merged
davidkyle merged 3 commits intoelastic:masterfrom
davidkyle:hlrc-put-calendar
Sep 14, 2018
Merged

HLRC: ML PUT Calendar#33362
davidkyle merged 3 commits intoelastic:masterfrom
davidkyle:hlrc-put-calendar

Conversation

@davidkyle
Copy link
Copy Markdown
Member

@davidkyle davidkyle commented Sep 3, 2018

For #29827

@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-core-infra

@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/ml-core

@dimitris-athanasiou
Copy link
Copy Markdown
Contributor

retest this please

Copy link
Copy Markdown
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

Some minor things and consistencies.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would think that the this.jobIds should be made immutable here, or at least create a new ArrayList from the passed in list.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For consistency, please use obj == null || getClass() != obj.getClass() for equality check.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See comment on the Calender#equals method.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be true. This is just a wrapper for the Calendar class whose parser ignores unknown fields.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

true as well here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this java-rest-high-snapshot... ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch. I based this on another doc which in turn must have been copied form the snapshot restore API. There were a a number of ML docs that had the same mis-naming

@davidkyle davidkyle force-pushed the hlrc-put-calendar branch 5 times, most recently from a3d49b5 to c8ff4ec Compare September 13, 2018 08:28
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

@davidkyle davidkyle merged commit b04faa0 into elastic:master Sep 14, 2018
@davidkyle davidkyle deleted the hlrc-put-calendar branch September 14, 2018 14:00
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 14, 2018
* master:
  Add script to cache dependencies (elastic#33726)
  [DOCS] Moves security reference to docs folder (elastic#33643)
  Cleanup assertions in global checkpoint listeners (elastic#33722)
  [CCR] Move ccr tests in core module back to ccr module (elastic#33711)
  HLRC: ML PUT Calendar (elastic#33362)
  [Tests] Fix randomization in StringTermsIT (elastic#33678)
  Pin TLS1.2 in SSLConfigurationReloaderTests
davidkyle added a commit that referenced this pull request Sep 18, 2018
@benwtrent benwtrent removed the :ml Machine learning label 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.

5 participants