Skip to content

Conversation

@gwbrown
Copy link
Contributor

@gwbrown gwbrown commented Oct 2, 2018

Also reworks the tests for the Set Policy response object to use the new
XContent testing capabilities, rather than having to implement
ToXContent on the response object itself.

Also reworks the tests for the Set Policy response object to use the new
XContent testing capabilities, rather than having to implement
ToXContent on the response object itself.
@gwbrown gwbrown added >non-issue :Data Management/ILM+SLM Index and Snapshot lifecycle management labels Oct 2, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM, left one comment about copyInstance


private SetIndexLifecyclePolicyRequest copyInstance(final SetIndexLifecyclePolicyRequest original) {
SetIndexLifecyclePolicyRequest copy = new SetIndexLifecyclePolicyRequest(original.policy(), original.indices());
copy.indicesOptions(original.indicesOptions());
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this need to copy the timeouts also?

Copy link
Contributor Author

@gwbrown gwbrown Oct 3, 2018

Choose a reason for hiding this comment

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

It probably should to be strictly correct, but this made me check (since obviously this doesn't make the equals() test fail) and as far as I can tell, no (or very, very few) *Request classes that inherit the timeout parameters from a superclass check them in the .equals() method. Is that something we should be concerned about?

(cc @hub-cap)

@gwbrown
Copy link
Contributor Author

gwbrown commented Oct 4, 2018

Closing this PR as this API is being removed by #34304

@gwbrown gwbrown closed this Oct 4, 2018
@gwbrown gwbrown deleted the ilm/update-set-policy branch December 7, 2018 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Data Management/ILM+SLM Index and Snapshot lifecycle management >non-issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants