-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
[TEST] Extend ShardPathTests for MDP #79015
Conversation
Add few more tests to ShardPathTests to cover the usage with multiple data paths. Relates to elastic#78485
Pinging @elastic/es-core-infra (Team:Core/Infra) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
.put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT).build(); | ||
IndexSettings idxSettings = IndexSettingsModule.newIndexSettings(randomAlphaOfLengthBetween(1, 10), indexSettings); | ||
|
||
ShardPath.deleteLeftoverShardDirectory(logger, env, lock, idxSettings, shardPaths -> assertEquals(path, shardPaths[0])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any assertion to make after this, like the dirs are deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 I totally lost my train of thought there 🙂. Let me add that.
Add few more tests to ShardPathTests to cover the usage with multiple data paths. Relates to elastic#78485
💚 Backport successful
|
Add few more tests to ShardPathTests to cover the
usage with multiple data paths.
I wasn't able to find any defects in the code related to multi paths, except that perhaps deleteLeftoverShardDirectory doesn't work with multiple paths yet. At the moment the only place where we call this delete method is with a single path use, so likely it isn't an issue for now.
Relates to #78485