clintropolis commented on a change in pull request #10724: URL: https://github.com/apache/druid/pull/10724#discussion_r553119211
########## File path: server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java ########## @@ -87,6 +88,8 @@ private final int maxSegmentsInNodeLoadingQueue; private final boolean pauseCoordination; + private static final EmittingLogger log = new EmittingLogger(CoordinatorDynamicConfig.class); Review comment: nit: I don't think this needs to be `EmittingLogger` since it isn't doing an alert anywhere ########## File path: server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java ########## @@ -96,7 +99,7 @@ public CoordinatorDynamicConfig( @JsonProperty("mergeBytesLimit") long mergeBytesLimit, @JsonProperty("mergeSegmentsLimit") int mergeSegmentsLimit, @JsonProperty("maxSegmentsToMove") int maxSegmentsToMove, - @JsonProperty("percentOfSegmentsToConsiderPerMove") double percentOfSegmentsToConsiderPerMove, + @JsonProperty("percentOfSegmentsToConsiderPerMove") Double percentOfSegmentsToConsiderPerMove, Review comment: I guess the alternative way to fix this instead of switching out the primitive would be to treat `0` as not configured and so use the default of `100`, though this way seems fine too, I think it just sticks out a bit because the rest of the numbery parameters are primitives. also nit: should annotate this parameter with `@Nullable` i missed the first PR, but any reason this is a double instead of integer similar to other percent based config `decommissioningMaxPercentOfMaxSegmentsToMove`? I guess there can be millions of segments so the numbers being dealt with here are a bit different (at least hopefully no one is trying to actually move on the scale of that many segments per coordinator run..) so maybe those decimal points really do make a difference, but otoh I can't really imagine operators configuring this much more granular than integer numbers, which I think was the reason we used integer for the other one iirc. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org