Jackie-Jiang 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_r372163260
##########
File path:
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -266,16 +267,42 @@ 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);
List<String> instanceTags = instanceConfig.getTags();
+ boolean toUpdateHelixRecord = false;
if (instanceTags == null || instanceTags.size() == 0) {
if
(ZKMetadataProvider.getClusterTenantIsolationEnabled(_helixManager.getHelixPropertyStore()))
{
- _helixAdmin.addInstanceTag(clusterName, instanceName,
TagNameUtils.getOfflineTagForTenant(null));
- _helixAdmin.addInstanceTag(clusterName, instanceName,
TagNameUtils.getRealtimeTagForTenant(null));
+ instanceConfig.addTag(TagNameUtils.getOfflineTagForTenant(null));
+ instanceConfig.addTag(TagNameUtils.getRealtimeTagForTenant(null));
} else {
- _helixAdmin.addInstanceTag(clusterName, instanceName,
UNTAGGED_SERVER_INSTANCE);
+ instanceConfig.addTag(UNTAGGED_SERVER_INSTANCE);
}
+ toUpdateHelixRecord = true;
+ }
+
+ // If the server config has both instance_id and host/port info, overwrite
the host/port info in zk. Without the
+ // overwrite, Helix will extract host/port from the instance_id instead of
use those in config.
+ // Use serverConf instead of _serverConf as the latter has been modified.
+ if (serverConf.containsKey(CONFIG_OF_INSTANCE_ID) &&
serverConf.containsKey(KEY_OF_SERVER_NETTY_HOST)) {
+ toUpdateHelixRecord = true;
+ // Internally, Helix use instanceId to derive Hostname and Port. To
decouple them, explicitly set the hostname/port
+ // field in zk.
+
instanceConfig.setHostName(_serverConf.getString(KEY_OF_SERVER_NETTY_HOST));
+
instanceConfig.setPort(Integer.toString(_serverConf.getInt(KEY_OF_SERVER_NETTY_PORT,
DEFAULT_SERVER_NETTY_PORT)));
+ }
Review comment:
Compare then set:
```
if (serverConf.containsKey(CONFIG_OF_INSTANCE_ID)) {
String hostName = _serverConf.getString(KEY_OF_SERVER_NETTY_HOST);
if (hostName != null && !hostName.equals(instanceConfig.getHostName())) {
instanceConfig.setHostName(hostName);
toUpdateHelixRecord = true;
}
String port = _serverConf.getString(KEY_OF_SERVER_NETTY_PORT);
if (port != null && !port.equals(instanceConfig.getPort())) {
instanceConfig.setPort(port);
toUpdateHelixRecord = true;
}
}
```
----------------------------------------------------------------
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]
