Skip to content

HLRC: ML PUT Calendar #33362

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 14, 2018
Merged

HLRC: ML PUT Calendar #33362

merged 3 commits into from
Sep 14, 2018

Conversation

@davidkyle
Copy link
Member

@davidkyle davidkyle commented Sep 3, 2018 •
edited
Loading

For #29827

@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 3, 2018

Pinging @elastic/es-core-infra

@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 3, 2018

Pinging @elastic/ml-core

@dimitris-athanasiou
Copy link
Contributor

dimitris-athanasiou commented Sep 3, 2018

retest this please

benwtrent
benwtrent reviewed Sep 4, 2018
View reviewed changes
Copy link
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.

client/rest-high-level/src/main/java/org/elasticsearch/client/ml/calendars/Calendar.java Outdated
Copy link
Member

@benwtrent benwtrent Sep 4, 2018

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
Member Author

@davidkyle davidkyle Sep 12, 2018

Choose a reason for hiding this comment

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

Good point

client/rest-high-level/src/main/java/org/elasticsearch/client/ml/calendars/Calendar.java Outdated
Copy link
Member

@benwtrent benwtrent Sep 4, 2018

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.

client/rest-high-level/src/main/java/org/elasticsearch/client/ml/calendars/ScheduledEvent.java Outdated
Copy link
Member

@benwtrent benwtrent Sep 4, 2018

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.

client/rest-high-level/src/test/java/org/elasticsearch/client/ml/PutCalendarResponseTests.java Outdated
Copy link
Member

@benwtrent benwtrent Sep 4, 2018

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.

client/rest-high-level/src/test/java/org/elasticsearch/client/ml/calendars/CalendarTests.java Outdated
Copy link
Member

@benwtrent benwtrent Sep 4, 2018

Choose a reason for hiding this comment

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

This should be true

...rest-high-level/src/test/java/org/elasticsearch/client/ml/calendars/ScheduledEventTests.java Outdated
Copy link
Member

@benwtrent benwtrent Sep 4, 2018

Choose a reason for hiding this comment

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

true as well here.

docs/java-rest/high-level/ml/put-calendar.asciidoc Outdated
Copy link
Member

@benwtrent benwtrent Sep 4, 2018

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
Member Author

@davidkyle davidkyle Sep 12, 2018

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
dimitris-athanasiou
dimitris-athanasiou approved these changes Sep 14, 2018
View reviewed changes
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

@davidkyle davidkyle merged commit b04faa0 into elastic:master Sep 14, 2018
@davidkyle davidkyle deleted the hlrc-put-calendar branch September 14, 2018 14:00
@elasticmachine elasticmachine mentioned this pull request Sep 14, 2018
Closed
98 tasks
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 14, 2018
@jasontedor
Merge branch 'master' into global-checkpoint-polling
cdc0e90 
* 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
@davidkyle
HLRC: ML PUT Calendar (#33362)
ba2465c
@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

Reviewers

@benwtrent benwtrent benwtrent left review comments

@dimitris-athanasiou dimitris-athanasiou dimitris-athanasiou approved these changes

Assignees

No one assigned

Labels

>enhancement v6.5.0 v7.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@davidkyle @elasticmachine @dimitris-athanasiou @benwtrent @colings86