Skip to content
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.

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

remove old doc placeholder and migrate ilm docs to top-level #34615

Merged
merged 17 commits into from
Oct 26, 2018

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Oct 18, 2018

Looks like we are restructuring the docs, this migrates ILM docs outside of the x-pack doc structure.

This is related to the initiative to move x-pack cleanup to ESRestTestCase #34530

@talevy talevy added >docs General docs changes :Data Management/ILM+SLM Index and Snapshot lifecycle management labels Oct 18, 2018
@talevy talevy requested a review from debadair October 18, 2018 18:53
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

LGTM but I'd like @debadair to look over the docs changes to make sure the new structure is in line with the way the docs will be structured going forward

@talevy
Copy link
Contributor Author

talevy commented Oct 19, 2018

thanks for the review @colings86

@gwbrown
Copy link
Contributor

gwbrown commented Oct 19, 2018

LGTM as well - and the extra cleanup of ILM will be nice.

@@ -441,6 +456,46 @@ private void waitForPendingRollupTasks() throws Exception {
});
}

private static void removePoliciesFromAllIndexes() throws IOException {
Response response = adminClient().performRequest(new Request("GET", "/_all"));
Copy link
Member

Choose a reason for hiding this comment

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

I think we'll usually have wiped the indices already. Do we need to do this in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, you're right. this is unnecessary

try {
adminClient().performRequest(new Request("DELETE", indexName + "/_ilm/"));
} catch (Exception e) {
// ok
Copy link
Member

Choose a reason for hiding this comment

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

OK because we're racing? Maybe we should add the ignore parameter to the request in that case.

Copy link
Member

Choose a reason for hiding this comment

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

We're really only ok with 404s here, right?

Response response = adminClient().performRequest(new Request("GET", "/_ilm"));
policies = ESRestTestCase.entityAsMap(response);
} catch (Exception e) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit broad.


try {
Response response = adminClient().performRequest(new Request("GET", "/_ilm"));
policies = ESRestTestCase.entityAsMap(response);
Copy link
Member

Choose a reason for hiding this comment

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

You are already in ESRestTestCase so you don't need the to reference the class.

try {
adminClient().performRequest(new Request("DELETE", "/_ilm/" + policyName));
} catch (Exception e) {
// ok
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as before about why this is ok.

@talevy
Copy link
Contributor Author

talevy commented Oct 22, 2018

thank you for the review @nik9000. I will clean up the exception handling and the likes. I will also be pending merging this PR until #34657 goes in so that I can properly modify the version checks for this to work when run in 6.x branches

try {
Response response = adminClient().performRequest(new Request("GET", "/_ilm"));
policies = entityAsMap(response);
} catch (ResponseException e) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is caught here because of #34881

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment referencing the issue and your intention to remove this once it is fixed. That'd be nice for someone reading the code after the fact I think.

@talevy
Copy link
Contributor Author

talevy commented Oct 25, 2018

#34657 was merged into master, so I will begin work to update and version-guard the deletion of policies to version of ES that contain ILM

@talevy talevy requested review from nik9000 and colings86 October 25, 2018 21:45
@talevy
Copy link
Contributor Author

talevy commented Oct 25, 2018

I will use 870655e for the backport to 6.x

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

LGTM

try {
Response response = adminClient().performRequest(new Request("GET", "/_ilm"));
policies = entityAsMap(response);
} catch (ResponseException e) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment referencing the issue and your intention to remove this once it is fixed. That'd be nice for someone reading the code after the fact I think.

@talevy
Copy link
Contributor Author

talevy commented Oct 25, 2018

thanks for the review @nik9000, the other PR was merged, so I will just remove the GET _ilm exception catching

@talevy
Copy link
Contributor Author

talevy commented Oct 26, 2018

test this please

@talevy talevy merged commit e737ea7 into elastic:index-lifecycle Oct 26, 2018
@talevy talevy deleted the ilm-move-docs branch October 26, 2018 19:19
talevy added a commit that referenced this pull request Oct 26, 2018
we are restructuring the docs, this migrates ILM docs outside of the x-pack doc structure.
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 >docs General docs changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants