-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Add coordinator dynamic config to limit the number of segments loaded per RunRules execution #12504
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
Closed
capistrant
wants to merge
21
commits into
apache:master
from
capistrant:maxSegmentsToLoad-dynamicConfig
Closed
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
cd4b629
Add coordinator dynamic config to limit the number of segments loaded…
fee9e66
fix spelling mistakes
a3d8c26
Update docs
f90f231
Run prettier --write on web console files
8c35880
Fix naming error in console for new config
df08bbf
update web-console snapshot
e0980bf
Merge branch 'master' into maxSegmentsToLoad-dynamicConfig
ee62efc
update naming of new dynamic config for clarity
5499eff
fixup CoordinatorDynamicConfigTest
b138fa2
fixup CoordinatorDynamicConfig
38b8a6a
re-work CooridnatorDynamicConfigTest to focus on testing the most int…
003a58b
Merge branch 'master' into maxSegmentsToLoad-dynamicConfig
02e04de
Merge branch 'master' into maxSegmentsToLoad-dynamicConfig
bb7ce62
Merge branch 'master' into maxSegmentsToLoad-dynamicConfig
426b6c8
Merge branch 'master' into maxSegmentsToLoad-dynamicConfig
6aac0ff
Merge branch 'master' into maxSegmentsToLoad-dynamicConfig
4ed6738
Merge branch 'master' into maxSegmentsToLoad-dynamicConfig
8b58895
Merge branch 'master' into maxSegmentsToLoad-dynamicConfig
29315f1
Merge branch 'master' into maxSegmentsToLoad-dynamicConfig
8bb4490
Merge branch 'master' into maxSegmentsToLoad-dynamicConfig
2095ba1
Merge branch 'master' into maxSegmentsToLoad-dynamicConfig
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
debugchoice seems a bit odd. On the one hand, it would seem reasonable to have defaults for values which are not provided. Ideally, if the user provides no value, then any existing value remains unchanged (though such a change is probably out of scope of this PR.)The idea is, as we add dynamic configs, the user should not have to first download all the existing settings, change the one of interest, and upload all of them. Just upload the one that needs to change and let Druid do the merge.
If we were to support the "values are optional" approach, then the user would need no warning when using it: doing so would be expected.
On the other hand, if we do require that the user specify all settings, including those added in the most recent release, then we should encourage people to update their dynamic config scripts with the new parameter, deciding on the default value they want. In that case, this error should be more than
DEBUGsince debug is often turned off. MaybeWARN?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 agree that the log is useless. Not sure why I continue to include it.. I had a former PR where I introduced a new configuration and initially had the log as a WARN to alert the operator. However, in consultation with the reviewer we decided that replacing a missing value with the default is "normal behavior" and thus shouldn't be logged as a warning. I don't recall exactly what my thoughts were in flipping to debug instead of just deleting.
IMO Druid should gracefully handle newly introduced configs on upgrade by having slipping in the new default. If the operator cares to use a non-default value for any new configs, they can POST their desired config spec to the API directly or use the Druid console to make the update as intended. Otherwise, Druid should just handle everything quietly on deserialization.
I created #11161 a long time ago when we identified that this check for null and set default as being clunky. I'm not sure why we don't leverage the
DynamicCoordinatorConfig#Builderto handle deserialization today. I could be missing something here that influenced the current serde, but nevertheless, I am prepping a separate PR to use the Builder instead of the actualCoordinatorDynamicConfigclass for deserialization. If that gets accepted, then this whole block of code could be tossed in the dumpster