-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Disallow multiple data paths for search nodes #6427
Disallow multiple data paths for search nodes #6427
Conversation
Signed-off-by: Xue Zhou <[email protected]>
Signed-off-by: Xue Zhou <[email protected]>
e4c972e
to
d1a7b87
Compare
@@ -774,4 +779,22 @@ Version getVersion() { | |||
version.set(Runtime.version()); | |||
BootstrapChecks.check(emptyContext, true, Collections.singletonList(check)); | |||
} | |||
|
|||
public void testMultipleDataPathCheck() throws NodeValidationException { |
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 think there should be 3 cases here:
- search node, 1 data path
- search node, 2 data paths
- data node, 2 data paths
You have case 2 covered, but not really the other two. They should be separate test methods as well because it is generally a best practice to make each test case test one thing whenever possible.
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.
Got it! Thanks for the reviews Andrew :)
CHANGELOG.md
Outdated
@@ -101,6 +101,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | |||
- Fix timeout error when adding a document to an index with extension running ([#6275](https://github.com/opensearch-project/OpenSearch/pull/6275)) | |||
- Handle translog upload during primary relocation for remote-backed indexes ([#5804](https://github.com/opensearch-project/OpenSearch/pull/5804)) | |||
- Batch translog sync/upload per x ms for remote-backed indexes ([#5854](https://github.com/opensearch-project/OpenSearch/pull/5854)) | |||
- Add bootstrap check to avoid search node has multiple data paths ([#6427](https://github.com/opensearch-project/OpenSearch/pull/6427)) |
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'd call this "Disallow multiple data paths for search nodes". Please update the commit message and PR title as well.
/** | ||
* Bootstrap check that if a search node contains multiple data paths | ||
*/ | ||
static class MultipleDataPathCheck implements BootstrapCheck { |
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.
Can this be private?
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.
Why should this class be private? I think it is reasonable to be default
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.
The answer to my question is "no" because it is referenced in the unit test, which I missed when I made this comment :)
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Looks like a legit failure @xuezhou25
|
); | ||
final List<BootstrapCheck> checks = Collections.singletonList(new BootstrapChecks.MultipleDataPathCheck()); | ||
final NodeValidationException e = expectThrows(NodeValidationException.class, () -> BootstrapChecks.check(context, true, checks)); | ||
assertThat(e.getMessage(), containsString("Having multiple data paths in the search role is not allowed")); |
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.
nit: assertThat
-> assertEquals
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 think it is preferable to use assertThat
due to #2503 (review)
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.
Isn't assertThat
deprecated?
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.
org.junit.Assert.assertThat()
is deprecated (which OpenSearchTestCase ultimately inherits from). org.hamcrest.MatcherAssert.assertThat
is not deprecated and is the intended replacement for that.
Signed-off-by: Xue Zhou <[email protected]>
…a_path # Conflicts: # CHANGELOG.md
Signed-off-by: Xue Zhou <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #6427 +/- ##
============================================
- Coverage 70.76% 70.70% -0.06%
+ Complexity 59093 59056 -37
============================================
Files 4802 4800 -2
Lines 282793 282719 -74
Branches 40782 40765 -17
============================================
- Hits 200111 199906 -205
- Misses 66271 66420 +149
+ Partials 16411 16393 -18
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
final List<BootstrapCheck> checks = Collections.singletonList(new BootstrapChecks.MultipleDataPathCheck()); | ||
final NodeValidationException e = expectThrows(NodeValidationException.class, () -> BootstrapChecks.check(context, true, checks)); | ||
assertThat(e.getMessage(), containsString("Having multiple data paths in the search node is not allowed")); | ||
} |
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.
Would this kind of private method help to remove some duplicate effort ?
public void performDataPathsCheck(String[] paths, String roleName) throws NodeValidationException {
final BootstrapContext context = createTestContext(
Settings.builder()
.putList(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), Collections.singletonList(roleName))
.putList(Environment.PATH_DATA_SETTING.getKey(), paths)
.build(),
Metadata.EMPTY_METADATA
);
final List<BootstrapCheck> checks = Collections.singletonList(new BootstrapChecks.MultipleDataPathCheck());
BootstrapChecks.check(emptyContext, true, checks);
}
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.
Looking pretty good! Just a couple minor issues.
public BootstrapCheckResult check(BootstrapContext context) { | ||
if (NodeRoleSettings.NODE_ROLES_SETTING.get(context.settings()).contains(DiscoveryNodeRole.SEARCH_ROLE) | ||
&& Environment.PATH_DATA_SETTING.get(context.settings()).size() > 1) { | ||
return BootstrapCheckResult.failure("Having multiple data paths in the search node is not allowed"); |
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.
Another nitpick on the wording, but something like "Multiple data paths are not allowed for search nodes" is probably a bit clearer and more direct.
Signed-off-by: Xue Zhou <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Xue Zhou <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Add bootstrap check to avoid search node has multiple data path Signed-off-by: Xue Zhou <[email protected]> (cherry picked from commit ceb3928) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Add bootstrap check to avoid search node has multiple data path (cherry picked from commit ceb3928) Signed-off-by: Xue Zhou <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
At node bootstrap, check if the node has search role and multiple data paths, if yes, fail the bootstrap.
Issues Resolved
resolve #6274
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.