-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Renaming the node role search to warm #17573
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
Conversation
Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
This is a straight rename correct? It does not resolve #17422 ? |
❌ Gradle check result for 835fd60: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
835fd60
to
a106fe2
Compare
Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
Yes, Do you want me to add a new issue for this? |
❌ Gradle check result for b0d026a: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17573 +/- ##
============================================
+ Coverage 72.48% 72.49% +0.01%
+ Complexity 65771 65752 -19
============================================
Files 5311 5311
Lines 304973 304973
Branches 44229 44229
============================================
+ Hits 221045 221085 +40
+ Misses 65830 65760 -70
- Partials 18098 18128 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Let's make sure this aligns with what @gbbafna mentioned here before merging. We'd generally want to do a deprecation cycle before just replacing a term like this. However, it does appear that |
@vinaykpud : Can we verify if we upgrade the cluster from 2.x to 3.0 with this change, searchable snapshots continue to work ? There might be few nuances here like changing the cluster-manager first . Want to make sure that rolling restarts works with this change |
server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/common/settings/ClusterSettings.java
Outdated
Show resolved
Hide resolved
Sure @gbbafna Performed a test to check the cluster upgrade scenario. Using opensearch-cluster-cdk I have setup cluster with 3 cluster manager and 2 data nodes.
Let me know if we missed anything here. |
So to summarize, you did a successful rolling upgrade from 2.19 to 3.0 but it required cm's to migrate first. Did you not have to update the routing pool logic for bwc? I don't see this in the pr. public static RoutingPool getNodePool(DiscoveryNode node) {
if (node.isWarmNode() || (node.isSearchNode() && (node.getVersion().before(Version.V_3_0_0)))) {
return REMOTE_CAPABLE;
}
return LOCAL_ONLY;
} |
@mch2 , No, I haven't updated the routing pool logic, it worked without adding any new logic. |
@vinaykpud No this logic would be on 3.0 cms not 2.x. I was thinking this would provide added safety for mixed cluster cases where we need to make alloc decisions and assign ss shards in that mixed state. Though in the normal upgrade case this isn't required. I think we can go ahead without this. |
Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
❕ Gradle check result for c6d46d2: UNSTABLE
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
* Renaming search node role to warm Signed-off-by: Vinay Krishna Pudyodu <[email protected]> * Added Changelog Signed-off-by: Vinay Krishna Pudyodu <[email protected]> * fixed failing tests Signed-off-by: Vinay Krishna Pudyodu <[email protected]> * fixed PR comments Signed-off-by: Vinay Krishna Pudyodu <[email protected]> --------- Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
* Renaming search node role to warm Signed-off-by: Vinay Krishna Pudyodu <[email protected]> * Added Changelog Signed-off-by: Vinay Krishna Pudyodu <[email protected]> * fixed failing tests Signed-off-by: Vinay Krishna Pudyodu <[email protected]> * fixed PR comments Signed-off-by: Vinay Krishna Pudyodu <[email protected]> --------- Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
In this PR we are renaming the existing Node "Search Role" to "Warm Role"
Description
This is done based on the decision taken as part of the discussion in this thread: #17422 (comment)
Related Issues
Related to #15306
Related to #17422
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.