suneet-s commented on a change in pull request #9302: Not use for CoordinatorRuleManager.rules URL: https://github.com/apache/druid/pull/9302#discussion_r373878635
########## File path: server/src/main/java/org/apache/druid/server/router/CoordinatorRuleManager.java ########## @@ -58,7 +57,7 @@ private final ObjectMapper jsonMapper; private final Supplier<TieredBrokerConfig> config; - private final AtomicReference<ConcurrentHashMap<String, List<Rule>>> rules; + private final AtomicReference<Map<String, List<Rule>>> rules; Review comment: @jihoonson I think we need some way to verify that this change works and will not break in the future. I understand it's not any worse than before, but I don't think we'll ever come back to add tests for this until we hit a bug - at which point it will be really hard to debug. It should be relatively quick to add those 2 tests so I think it's worth it. If there's a fast follow, that should be ok. I'll leave it to you for final decision on whether this should be a blocker or not (I'm not sure what the apache rules are), but I think debugging this if someone changes the code to mutate the map would be very tough - I'd strongly recommend we add tests here to prevent that pain in the future. ---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
