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

ILM: Moving full inspection of all indices to MasterFailOver condition #87616

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

PDTCCLF
Copy link

@PDTCCLF PDTCCLF commented Jun 13, 2022

This is my attempt at solving issue #80407. As per the description of the issue, the costly loop that inspects all indices of a cluster state is no longer being executed by default. A full inspection only happens when an exception occurs to the master, inspired by SnapshotService.

@elasticsearchmachine elasticsearchmachine added v8.4.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Jun 13, 2022
@gwbrown gwbrown added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label Jun 13, 2022
@PDTCCLF PDTCCLF marked this pull request as ready for review June 19, 2022 21:53
@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Jun 19, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@@ -208,9 +212,9 @@ void onMaster(ClusterState clusterState) {
} catch (Exception e) {
if (logger.isTraceEnabled()) {
logger.warn(
() -> format(
new ParameterizedMessage(
Copy link
Contributor

@joegallo joegallo Jun 24, 2022

Choose a reason for hiding this comment

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

Thise lambda to ParameterizedMessage looks like a bad merge to me, please undo it.

@@ -220,9 +224,9 @@ void onMaster(ClusterState clusterState) {
);
} else {
logger.warn(
() -> format(
new ParameterizedMessage(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

@@ -243,6 +247,7 @@ void onMaster(ClusterState clusterState) {
}
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove errant whitespace change.

Copy link
Contributor

@joegallo joegallo left a comment

Choose a reason for hiding this comment

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

Beyond my other issues, the code associated with this PR doesn't compile. That's a pretty big red flag.

@PDTCCLF
Copy link
Author

PDTCCLF commented Jun 30, 2022

Beyond my other issues, the code associated with this PR doesn't compile. That's a pretty big red flag.

I see. I will try to fix the details you mentioned and make sure it does compile. I'm sorry, I believed it had passed gradle check, but I was mistaken.

@joegallo
Copy link
Contributor

No worries, best of luck.

The previous version of the PR did not compile due to a variable that was being called on MasterFailOver, but was not set. I fixed this problem, plus the mentioned errant whitespace and bad merge.
@elasticsearchmachine elasticsearchmachine changed the base branch from master to main July 22, 2022 23:06
@mark-vieira mark-vieira added v8.5.0 and removed v8.4.0 labels Jul 27, 2022
@csoulios csoulios added v8.6.0 and removed v8.5.0 labels Sep 21, 2022
@gmarouli
Copy link
Contributor

Hi @PDTCCLF, thank you for working on this. I see there hasn't been any progress in this PR since July, I hope you do not mind if I give it a try myself. Let me know if you are still actively working on it. Then I will look for something else.

@PDTCCLF
Copy link
Author

PDTCCLF commented Oct 31, 2022

Hi @PDTCCLF, thank you for working on this. I see there hasn't been any progress in this PR since July, I hope you do not mind if I give it a try myself. Let me know if you are still actively working on it. Then I will look for something else.

Not at all. You can work on it. Thanks for asking.

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 >enhancement external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Data Management Meta label for data/management team v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.