paul-rogers commented on code in PR #12504:
URL: https://github.com/apache/druid/pull/12504#discussion_r887380372
##########
server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java:
##########
@@ -195,6 +203,22 @@ public CoordinatorDynamicConfig(
"maxNonPrimaryReplicantsToLoad must be greater than or equal to 0."
);
this.maxNonPrimaryReplicantsToLoad = maxNonPrimaryReplicantsToLoad;
+
+ if (maxSegmentsToLoad == null) {
+ log.debug(
Review Comment:
The `debug` choice 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
`DEBUG` since debug is often turned off. Maybe `WARN`?
##########
server/src/test/java/org/apache/druid/server/http/CoordinatorDynamicConfigTest.java:
##########
@@ -89,6 +89,7 @@ public void testSerde() throws Exception
9,
false,
false,
+ Integer.MAX_VALUE,
Review Comment:
There are a pile of these. Should we introduce a builder or some other way
to reduce redundancy?
##########
docs/configuration/index.md:
##########
@@ -898,7 +898,8 @@ A sample Coordinator dynamic config JSON object is shown
below:
"decommissioningMaxPercentOfMaxSegmentsToMove": 70,
"pauseCoordination": false,
"replicateAfterLoadTimeout": false,
- "maxNonPrimaryReplicantsToLoad": 2147483647
+ "maxNonPrimaryReplicantsToLoad": 2147483647,
+ "maxSegmentsToLoad": 2147483647
Review Comment:
Should the name be more specific? "Max segments to load", by itself, sounds
like "the maximum number of segments which the coordinator will load ...
period" -- an upper limit on the number of loaded segments overall. This then
raises the question, "but, what happens to the others?"
From the description, it sounds like this is "per Coordination run". So,
should the name be something like `maxSegmentsPerCoordination`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
