Skip to content

[hlrc] add index templates exist API#36132

Merged
andyb-elastic merged 4 commits intoelastic:masterfrom
andyb-elastic:feature-hlrc-index-template-exists
Dec 10, 2018
Merged

[hlrc] add index templates exist API#36132
andyb-elastic merged 4 commits intoelastic:masterfrom
andyb-elastic:feature-hlrc-index-template-exists

Conversation

@andyb-elastic
Copy link
Copy Markdown
Contributor

This commit adds support for the index templates exist API, using the
existing GetIndexTemplatesRequest request object. Also adds links to the
index template APIs which are supported in the client

For #27205

This commit adds support for the index templates exist API, using the
existing GetIndexTemplatesRequest request object. Also adds links to the
index template APIs which are supported in the client
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-core-features

Copy link
Copy Markdown
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

Only real issue is that you dont have a request class thats not a server based request class. Please duplicate it and remove all the server junk from it and this should be good to go.

throw new IllegalArgumentException("Must provide at least one index template name");
}

if (Arrays.stream(getIndexTemplatesRequest.names()).anyMatch(indexTemplate -> Strings.hasText(indexTemplate) == false)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you create a copy of GetIndexTemplatesRequest and move the validation into that instead? Put it in o.e.client.indices. Also remove all of the to/from transport action stuff, and do your best to make constructor params final.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will do

@andyb-elastic
Copy link
Copy Markdown
Contributor Author

@hub-cap I pushed changes for a new GetIndexTemplatesRequest in the client project, and added another request type for the index templates exist API because it has the added constraint of requiring a name parameter

Copy link
Copy Markdown
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

Awesome, 2 nit/question, hopefully they work out. But I dont need to see this again, the new client side object looks way better than the old one. Great job.

/**
* @return the timeout for waiting for the master node to respond
*/
public TimeValue getMasterNodeTimeout() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

check out TimedRequest, see if it will work here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The API doesn't support the timeout parameter that TimedRequest has - I figured it was less bad to duplicate it here vs. have the timeout methods unsupported in this class

final Optional<ValidationException> parent = super.validate();
final ValidationException validationException = parent.orElse(new ValidationException());
if (names().isEmpty()) {
validationException.addValidationError("must provide at least one name");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is it possible to move this to the constructor instead? the whole validate() thing was a relic that i brought forward for back compat on the existing internal APIs in HLRC. We prefer it in the constructor so it can bomb out with a stack trace that shows where it was set, rather than some weird place after the fact (which is what validate does)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For sure, completely agree

@andyb-elastic andyb-elastic merged commit 10938e5 into elastic:master Dec 10, 2018
andyb-elastic added a commit to andyb-elastic/elasticsearch that referenced this pull request Dec 10, 2018
This commit adds support for the index templates exist API, creating
new client-side request types for that API and the get index
templates API. Also adds links in hlrc docs to pages for supported
index template APIs
andyb-elastic added a commit that referenced this pull request Dec 10, 2018
This commit adds support for the index templates exist API, creating
new client-side request types for that API and the get index
templates API. Also adds links in hlrc docs to pages for supported
index template APIs
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.

4 participants