Skip to content

Adds ILMClient for use with transport client#33357

Merged
colings86 merged 3 commits intoelastic:index-lifecyclefrom
colings86:ilm/transport-client
Sep 5, 2018
Merged

Adds ILMClient for use with transport client#33357
colings86 merged 3 commits intoelastic:index-lifecyclefrom
colings86:ilm/transport-client

Conversation

@colings86
Copy link
Copy Markdown
Contributor

@colings86 colings86 commented Sep 3, 2018

No description provided.

@colings86 colings86 added review :Data Management/ILM+SLM DO NOT USE. Use ":StorageEngine/ILM" or ":Distributed Coordination/SLM" instead. labels Sep 3, 2018
@colings86 colings86 self-assigned this Sep 3, 2018
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Sep 3, 2018

Pinging @elastic/es-core-infra

@elasticmachine elasticmachine mentioned this pull request Sep 3, 2018
Closed
AthenaEryma
AthenaEryma approved these changes Sep 4, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@AthenaEryma AthenaEryma left a comment

Choose a reason for hiding this comment

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

Does this need tests? I think I recall a conversation where it was noted that transport client stuff isn't really tested, but just asking in case.

Otherwise, LGTM.

@dakrone
Copy link
Copy Markdown
Member

dakrone commented Sep 4, 2018

Are we going all async listener for this? In our other clients (like IndicesAdminClient for instance) we have versions that return ActionFuture so someone can do ExplainLifecycleResponse resp = client.explainLifecycle(req).get();

@colings86
Copy link
Copy Markdown
Contributor Author

colings86 commented Sep 4, 2018

Good point @dakrone I'll add the sync methods

@colings86
Copy link
Copy Markdown
Contributor Author

colings86 commented Sep 4, 2018

@dakrone I pushed a commit with the sync methods

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

I believe this needs to be added to XPackClient similar to how we have the other MonitoringClient, WatcherClient, etc, that way it is accessible rather than instantiating it ourselves

dakrone
dakrone approved these changes Sep 4, 2018
View reviewed changes
Copy link
Copy Markdown
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, thanks Colin

@colings86 colings86 merged commit a7b5f2c into elastic:index-lifecycle Sep 5, 2018
@colings86 colings86 deleted the ilm/transport-client branch September 5, 2018 11:50
colings86 added a commit that referenced this pull request Sep 5, 2018
@colings86
Adds ILMClient for use with transport client (#33357)
6fe05bf 
* Adds ILMClient for use with transport client

* add sync methods

* adds ILMClient to XPackClient
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dakrone dakrone dakrone approved these changes

@talevy talevy Awaiting requested review from talevy

@jasontedor jasontedor Awaiting requested review from jasontedor

+1 more reviewer

@AthenaEryma AthenaEryma AthenaEryma approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@colings86 colings86

Labels

:Data Management/ILM+SLM DO NOT USE. Use ":StorageEngine/ILM" or ":Distributed Coordination/SLM" instead.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@colings86 @elasticmachine @dakrone @AthenaEryma