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

Make clean up files step configurable for peer-recovery of replicas #92490

Merged

Conversation

tlrx
Copy link
Member

@tlrx tlrx commented Dec 21, 2022

Skip the "clean up and verify" step at the end of files based peer-recovery for replicas.

@tlrx tlrx added >enhancement :Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. v8.7.0 labels Dec 21, 2022
@elasticsearchmachine
Copy link
Collaborator

Hi @tlrx, I've created a changelog YAML for you.

@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team label Dec 21, 2022
@@ -495,7 +495,9 @@ public void cleanFiles(
final Store store = store();
store.incRef();
try {
store.cleanupAndVerify("recovery CleanFilesRequestHandler", sourceMetadata);
if (DiscoveryNode.isStateless(indexShard.indexSettings().getSettings()) == false || indexShard.routingEntry().primary()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to pass in the node settings, not the index settings to use that method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Documentation of IndexSettings#getSettings mentions:

Returns the settings for this index. These settings contain the node and index level settings where
settings that are specified on both index and node level are overwritten by the index settings.

Copy link
Member Author

@tlrx tlrx Dec 21, 2022

Choose a reason for hiding this comment

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

So I think we're good here: I verified and it does contain the node settings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, it will work, but seems more intuitive to use indexShard.indexSettings().getNodeSettings()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh sorry, I misunderstood your previous comment. Sure, getNodeSettings is more appropriate, thanks for pointing this.

Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM.

@tlrx tlrx added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Dec 21, 2022
@elasticsearchmachine elasticsearchmachine merged commit 562fdc9 into elastic:main Dec 21, 2022
@tlrx tlrx deleted the do-not-clean-up-files-for-replica branch December 21, 2022 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. >enhancement Team:Distributed Meta label for distributed team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants