-
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
Add search node repurpose commands #6517
Add search node repurpose commands #6517
Conversation
Gradle Check (Jenkins) Run Completed with:
|
bc3e16e
to
fa1dac0
Compare
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 #6517 +/- ##
=========================================
Coverage 70.79% 70.80%
- Complexity 59121 59166 +45
=========================================
Files 4804 4804
Lines 283141 283210 +69
Branches 40815 40841 +26
=========================================
+ Hits 200462 200531 +69
+ Misses 66229 66208 -21
- Partials 16450 16471 +21
... and 502 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
fa1dac0
to
334f0ca
Compare
Gradle Check (Jenkins) Run Completed with:
|
processNoClusterManagerNoDataNode(terminal, dataPaths, env); | ||
} else { | ||
processClusterManagerNoDataNode(terminal, dataPaths, env); | ||
if (DiscoveryNode.isDataNode(env.settings()) == false) { |
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.
What if both isDataNode
and isSearchNode
are false? This will only execute the first branch of the if statement. Is that correct?
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 idea here was that it would operate in a step fashion, because the errors are thrown individually - https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/env/NodeEnvironment.java#L386-L396
The clients can perform the repurposing one at a time.
WDYT?
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.
So lets say I have a node that has been used as both a data
and a search
node and I want to repurpose it to be a cluster manager node. Does that mean I'd have to run the repurpose command twice?
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.
Yep, since the conditions are currently laid out differently within node startup.
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.
But node startup doesn't have anything to do with this repurpose command though, right? If I run this command on a host that had multiple roles but now no longer has any of them, I would expect it to do the right thing and clear everything out. Is that difficult or not possible for some reason?
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.
It's not difficult, just a bunch of combinatorial logic, which I was trying to avoid. I can add it in, shouldn't take a lot of time.
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.
Does it have to be combinatorial? Like why can't it be:
if not data role:
clean up data if exists
if not search role:
clean up search if exists
...
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.
Updated.
03a1fbb
to
e26cf31
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
e26cf31
to
7b4ca8d
Compare
Gradle Check (Jenkins) Run Completed with:
|
7b4ca8d
to
4d8a6b4
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Kunal Kotwani <[email protected]>
4d8a6b4
to
8afcc8a
Compare
Gradle Check (Jenkins) Run Completed with:
|
if (repurposeData && repurposeSearch) { | ||
terminal.println("Node successfully repurposed to no-cluster-manager, no-data and no-search."); | ||
} else if (repurposeData) { | ||
terminal.println("Node successfully repurposed to no-cluster-manager and no-data."); | ||
} else if (repurposeSearch) { | ||
terminal.println("Node successfully repurposed to no-cluster-manager and no-search."); | ||
} |
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.
Seems like you could avoid this explosion of cases with something like:
roleText.add("no-cluster-manager");
if (repurposeData) roleText.add("no-data");
if (repurposeSearch) roleText.add("no-search");
terminal.println("Node successfully repurposed to: " + String.join(",", roleText));
Same applies in the other cases though might require more refactoring. What do you think?
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 thought of doing that, but this makes it more readable and traceable. Given its use case and frequency of use, I think we would rather benefit from this approach.
Signed-off-by: Kunal Kotwani <[email protected]> (cherry picked from commit 71a76da) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit 71a76da) Signed-off-by: Kunal Kotwani <[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>
Signed-off-by: Kunal Kotwani <[email protected]> Signed-off-by: Mingshi Liu <[email protected]>
Description
Issues Resolved
[List any issues this PR will resolve]
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.