capistrant commented on code in PR #12504:
URL: https://github.com/apache/druid/pull/12504#discussion_r891415269
##########
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:
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#Builder` to 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 actual
`CoordinatorDynamicConfig` class for deserialization. If that gets accepted,
then this whole block of code could be tossed in the dumpster
--
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]
