-
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 new role for Offline Nodes #13613
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Varun Bansal <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13613 +/- ##
============================================
+ Coverage 71.42% 71.47% +0.05%
- Complexity 59978 61054 +1076
============================================
Files 4985 5052 +67
Lines 282275 287155 +4880
Branches 40946 41608 +662
============================================
+ Hits 201603 205258 +3655
- Misses 63999 64946 +947
- Partials 16673 16951 +278 ☔ View full report in Codecov by Sentry. |
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.
Do we plan to make the changelog entry with this PR or once the feature has all code changes in?
public static boolean isOfflineNode(Settings settings) { | ||
return hasRole(settings, DiscoveryNodeRole.OFFLINE_ROLE); | ||
} | ||
|
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.
DiscoveryNode
has instance methods for these too. Do we need one for the offline role?
@@ -391,6 +392,20 @@ public NodeEnvironment(Settings settings, Environment environment, IndexStoreLis | |||
ensureNoFileCacheData(fileCacheNodePath); | |||
} | |||
|
|||
if (DiscoveryNode.isOfflineNode(settings) && DiscoveryNode.getRolesFromSettings(settings).size() > 1) { |
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.
Please could you help add tests for this?
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.
tests are pending. will be working on adding those
I am guessing the intent of an offline node is to execute offline tasks. The naming is confusing. A node that's offline is ... offline. I think a better name could be a "worker" or "task" node that is capable of executing tasks, and doesn't serve other purpose than that. The node itself is not offline. |
Thanks for the feedback @dblock . I agree Offline does create some confusion and did spend some time initially to think of a better name but didn't have much luck. But let me give it another shot and come up with a better keyword than "Offline". If you are interested in getting more context around Offline Nodes, here is a detailed design doc #13554 and a PR introducing the base library #13574 |
This PR is stalled because it has been open for 30 days with no activity. |
Description
[Describe what this change achieves]
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
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.