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

Prioritise recovery of system index shards #62640

Conversation

pugnascotia
Copy link
Contributor

Closes #61660. When ordering shard for recovery, ensure system index shards are ordered first so that their recovery will be started first.

Note that I rewrote PriorityComparatorTests to use IndexMetadata instead of its local IndexMeta POJO.

Closes elastic#61660. When ordering shard for recovery, ensure system index
shards are ordered first.
@pugnascotia pugnascotia added >enhancement :Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. v8.0.0 v7.10.0 labels Sep 18, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Recovery)

@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team label Sep 18, 2020
Copy link
Member

@jaymode jaymode 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 let's also get someone from the distributed team to review

@pugnascotia
Copy link
Contributor Author

@elasticmachine update branch

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

LGTM - I left some minor comments, feel free to use them or not

}
// else sometimes just use the defaults

indices[i] = IndexMetadata.builder("idx_2015_04_" + String.format(Locale.ROOT, "%02d", i))
Copy link
Member

Choose a reason for hiding this comment

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

nit: since i can span to 99 using a prefix like "idx_" makes more sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting just "idx_" + i?

I'm not sure what the best thing to do here is. The comparator class just does a compareTo on the index names, so in a sense it doesn't matter what index names we use, so long as the result is sorted the way we expect. However, the intention of the sort is that newer indices, according to dates in the index names, are sorted first. So I wonder if it's worth preserving a date-like pattern in the index names for that reason? What do you think?

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 that we should test what the code is effectively doing, ie alphabetically sorting by index names. Applying this to index names that contain a date is sort of a workaround as most of the time SETTING_CREATION_DATE should be present. Prioritizing index-0002 over index-0001 is also a thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I swapped it for just "idx_%04d". Any better?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@pugnascotia
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

@pugnascotia pugnascotia merged commit 39a6dec into elastic:master Sep 22, 2020
@pugnascotia pugnascotia deleted the 61660-prioritize-recovery-of-system-indices branch September 22, 2020 14:47
pugnascotia added a commit that referenced this pull request Sep 22, 2020
Closes #61660. When ordering shard for recovery, ensure system index shards are
ordered first so that their recovery will be started first.

Note that I rewrote PriorityComparatorTests to use IndexMetadata instead of its
local IndexMeta POJO.
@pugnascotia
Copy link
Contributor Author

Backported to 7.x in 3f856d1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. >enhancement Team:Distributed Meta label for distributed team v7.10.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prioritize recovery of system indices
5 participants