chenboat commented on a change in pull request #4995: Decouple server instance
id with hostname/port config.
URL: https://github.com/apache/incubator-pinot/pull/4995#discussion_r372008333
##########
File path:
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -266,7 +266,7 @@ private void updateInstanceConfigInHelix(Map<String,
String> props) {
_helixAdmin.setConfig(scope, props);
}
- private void addInstanceTagIfNeeded(String clusterName, String instanceName)
{
+ private void updateInstanceConfigIfNeeded(String clusterName, String
instanceName, Configuration serverConf) {
InstanceConfig instanceConfig = _helixAdmin.getInstanceConfig(clusterName,
instanceName);
Review comment:
What is the use of a boolean flag here? We have to read from the zk anyway
because to update tags needs to the current number of tags. As for writing to
zk, the current boolean conditions already check for cases we do not need to
write to zk.
There could be an optimization to combine two zk writes into one -- but
there seems to be many logics specific to instance tag update already. I wonder
if it is worthwhile to do this optimization. Instance updates to zk are start
only events and the performance impact here should be fine, right?
----------------------------------------------------------------
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]
