FrankChen021 commented on code in PR #19856: URL: https://github.com/apache/druid/pull/19856#discussion_r3699120351
########## services/src/main/java/org/apache/druid/server/router/JavaScriptTieredBrokerSelectorStrategy.java: ########## @@ -53,9 +53,11 @@ public JavaScriptTieredBrokerSelectorStrategy( } @Override - public Optional<String> getBrokerServiceName(TieredBrokerConfig config, Query query) + public synchronized Optional<String> getBrokerServiceName(TieredBrokerConfig config, Query query) Review Comment: Although Codex reports 'LGTM', I have different opinons. If you look at the usage of compileSelectorFunction, there's another place that uses it, and the 'synchronized' is not added. So adding 'synchronized' to caller is not good, we may forget to add it in new places. As this is due to script engine layer problem, I think it's the `compileSelectorFunction` that ensure the returned fnSelector is concurrent safe version. One way is that we can use dynamic proxy inside compileSelectorFunction to wrap existing object and forward the call in a synchronized block -- 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]
