Add high level rest client support for SetIndexLifecyclePolicy#32443
Merged
dakrone merged 4 commits intoelastic:index-lifecyclefrom Jul 30, 2018
Merged
Conversation
This adds HLRC support for the ILM operation of setting an index's lifecycle policy. It also includes extracting and renaming a number of classes (like the request and response objects) as well as the addition of a new `IndexLifecycleClient` for the HLRC. This is a prerequisite to making the `index.lifecycle.name` setting internal only, because we require a dedicated REST endpoint to change the policy, and our tests currently set this setting with the REST client multiple places. A subsequent PR will change the setting to be internal and move those uses over to this new API. This misses some links to the documentation because I don't think ILM has any documentation available yet. Relates to elastic#29827 and elastic#29823
Collaborator
|
Pinging @elastic/es-core-infra |
colings86
approved these changes
Jul 30, 2018
Contributor
colings86
left a comment
There was a problem hiding this comment.
Left a very minor comment but LGTM
| public void testSetIndexLifecyclePolicy() throws Exception { | ||
| String policy = randomAlphaOfLength(10); | ||
|
|
||
| // TODO: convert this to using the high level client once there are APIs for it |
Contributor
There was a problem hiding this comment.
Nit: Could you make this a NORELEASE since we want to do this before we release rather than just at some point?
…rc-support-for-set-index-policy
hub-cap
reviewed
Jul 30, 2018
Contributor
hub-cap
left a comment
There was a problem hiding this comment.
I cannot tell if you are renaming or renaming and adding content to a lot of these classes so its hard to give a good review. But of the things i did review, good job on the tests being added (the abstract streamables) and most of the client work itself was good. Minor nits.
| static Request setIndexLifecyclePolicy(SetIndexLifecyclePolicyRequest setPolicyRequest) { | ||
| Request request = new Request(HttpPut.METHOD_NAME, | ||
| new EndpointBuilder() | ||
| .addCommaSeparatedPathParts(setPolicyRequest.indices()) |
Contributor
There was a problem hiding this comment.
Pls be sure this is not null. Other converters do a null check and return and give this addCommaSeparatedPathParts a empty array if need be. check forceMerge for an example
| public void testSetIndexLifecyclePolicy() throws Exception { | ||
| String policy = randomAlphaOfLength(10); | ||
|
|
||
| // TODO: NORELEASE convert this to using the high level client once there are APIs for it |
Contributor
There was a problem hiding this comment.
lol i found this out the hard way by not starging with "put/create" in the client too :)
| public void testSetIndexLifecyclePolicy() throws Exception { | ||
| SetIndexLifecyclePolicyRequest req = new SetIndexLifecyclePolicyRequest(); | ||
| String policyName = randomAlphaOfLength(10); | ||
| String[] indices = randomIndicesNames(0, 10); |
Contributor
There was a problem hiding this comment.
if this can be null maybe do a rarely() and assign to null.
jasontedor
pushed a commit
that referenced
this pull request
Aug 17, 2018
This adds HLRC support for the ILM operation of setting an index's lifecycle policy. It also includes extracting and renaming a number of classes (like the request and response objects) as well as the addition of a new `IndexLifecycleClient` for the HLRC. This is a prerequisite to making the `index.lifecycle.name` setting internal only, because we require a dedicated REST endpoint to change the policy, and our tests currently set this setting with the REST client multiple places. A subsequent PR will change the setting to be internal and move those uses over to this new API. This misses some links to the documentation because I don't think ILM has any documentation available yet. Relates to #29827 and #29823
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds HLRC support for the ILM operation of setting an index's lifecycle
policy.
It also includes extracting and renaming a number of classes (like the request
and response objects) as well as the addition of a new
IndexLifecycleClientfor the HLRC. This is a prerequisite to making the
index.lifecycle.namesetting internal only, because we require a dedicated REST endpoint to change
the policy, and our tests currently set this setting with the REST client
multiple places. A subsequent PR will change the setting to be internal and move
those uses over to this new API.
This misses some links to the documentation because I don't think ILM has any
documentation available yet.
Relates to #29827 and #29823