Skip to content

HLRC: Add ILM Retry #33990

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 15 commits into from
Oct 19, 2018
Merged

HLRC: Add ILM Retry #33990

merged 15 commits into from
Oct 19, 2018

Conversation

original-brownbear
Copy link
Member

@original-brownbear original-brownbear commented Sep 24, 2018 •
edited
Loading

  • Unfortunately I only found an easy way to test the negative case here (retry fails). We have the same issue in the rest API tests where we also seem to only test the failure though.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

hub-cap
hub-cap suggested changes Sep 28, 2018
View reviewed changes
Copy link
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.

I dont see any docs or request converter tests. I understand that you wouldn't need to do response tests since there is no response. Other than that, it looks good.

client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java
@@ -690,6 +692,19 @@ static Request explainLifecycle(ExplainLifecycleRequest explainLifecycleRequest)
return request;
}

static Request retryLifecycle(RetryLifecyclePolicyRequest retryLifecyclePolicyRequest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be in some IndexLifecycleRequestConverters class. i dont think it exists yet.

Copy link
Contributor

@gwbrown gwbrown Sep 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, ILM stuff hasn't been moved to a separate RequestConverters class yet. I'd say this is okay for this PR since we'll do a mass move at some point before merging anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

works for me

hub-cap
hub-cap reviewed Sep 28, 2018
View reviewed changes
client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java
@@ -1075,6 +1090,11 @@ EndpointBuilder addCommaSeparatedPathParts(String[] parts) {
return this;
}

EndpointBuilder addCommaSeparatedPathParts(List<String> parts) {
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/elastic/elasticsearch/blob/master/client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java#L966

Copy link
Member Author

Choose a reason for hiding this comment

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

I just added this to save a toArray() call here, seemed cleaner to just add that API, but I don't have a strong opinion here :)

@original-brownbear
Copy link
Member Author

@hub-cap added request converter test :)
What kind of docs are you looking for?

@hub-cap
Copy link
Contributor

hub-cap commented Oct 2, 2018

@original-brownbear have a look at the other PRs. They add a asciidoc page as well as a DocumentationIT test to back the asciidoc for the API.

@colings86 colings86 mentioned this pull request Oct 4, 2018
Closed
13 tasks
@original-brownbear
Copy link
Member Author

@hub-cap got it thanks, will add docs shortly :)

talevy
talevy approved these changes Oct 17, 2018
View reviewed changes
Copy link
Contributor

@talevy talevy left a comment

Choose a reason for hiding this comment

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

LGTM

@hub-cap I agree with you re docs, but ILM is lacking on that front in general, and I have a task to fix that. So, I will continue with HLRC ILM docs outside of this PR

@hub-cap
Copy link
Contributor

hub-cap commented Oct 17, 2018

as long as yall do it, im fine not blocking this.

@original-brownbear
Copy link
Member Author

@talevy @hub-cap there's some test failures here but I get the same with the index-lifecycle branch. I'll hold off on merging here until that's fixed, if you need/want this merged earlier just merge :)

@talevy
Copy link
Contributor

talevy commented Oct 18, 2018

thanks @original-brownbear, we're looking into it

@original-brownbear
Copy link
Member Author

@talevy seems you guys fixed it thanks green now after merging in index-lifecycle => merging!

@original-brownbear original-brownbear merged commit 910e974 into elastic:index-lifecycle Oct 19, 2018
@original-brownbear original-brownbear deleted the hlrc-ilm-retry-step branch October 19, 2018 13:04
gwbrown pushed a commit that referenced this pull request Oct 25, 2018
@original-brownbear @gwbrown
HLRC: Add ILM Retry (#33990)
88c7b39 
* HLRC: Add ILM Retry

* Relates #33100
@colings86 colings86 added >non-issue :Data Management/ILM+SLM Index and Snapshot lifecycle management v6.6.0 and removed :Data Management/ILM+SLM Index and Snapshot lifecycle management >enhancement v6.5.0 labels Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gwbrown gwbrown gwbrown left review comments

@talevy talevy talevy approved these changes

@hub-cap hub-cap hub-cap approved these changes

Assignees
No one assigned
Labels
:Data Management/ILM+SLM Index and Snapshot lifecycle management >non-issue v6.6.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@original-brownbear @elasticmachine @hub-cap @talevy @gwbrown @colings86