[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #7064: Allow updating controller and broker helix hostname

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r654596456



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -139,12 +139,13 @@ public HelixBrokerStarter(PinotConfiguration brokerConf) throws Exception {
     if (brokerHost == null) {
       brokerHost = _brokerConf.getProperty(Helix.SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtils
           .getHostnameOrAddress() : NetUtils.getHostAddress();
+      brokerConf.setProperty(Broker.CONFIG_OF_BROKER_HOSTNAME, brokerHost);

Review comment:
       Suggest making `brokerHost` a member variable instead of backfilling it into the config. Ideally the config should not be modified

##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -266,65 +264,21 @@ private void registerServiceStatusHandler() {
   }
 
   private void updateInstanceConfigIfNeeded(String host, int port) {
-    InstanceConfig instanceConfig = _helixAdmin.getInstanceConfig(_helixClusterName, _instanceId);
-    boolean needToUpdateInstanceConfig = false;
-
-    // Add default instance tags if not exist
-    List<String> instanceTags = instanceConfig.getTags();
-    if (instanceTags == null || instanceTags.size() == 0) {
-      if (ZKMetadataProvider.getClusterTenantIsolationEnabled(_helixManager.getHelixPropertyStore())) {
-        instanceConfig.addTag(TagNameUtils.getOfflineTagForTenant(null));
-        instanceConfig.addTag(TagNameUtils.getRealtimeTagForTenant(null));
-      } else {
-        instanceConfig.addTag(Helix.UNTAGGED_SERVER_INSTANCE);
-      }
-      needToUpdateInstanceConfig = true;
-    }
-
-    // Update host and port if needed
-    if (!host.equals(instanceConfig.getHostName())) {
-      instanceConfig.setHostName(host);
-      needToUpdateInstanceConfig = true;
-    }
-    String portStr = Integer.toString(port);
-    if (!portStr.equals(instanceConfig.getPort())) {
-      instanceConfig.setPort(portStr);
-      needToUpdateInstanceConfig = true;
-    }
-
-    // Update instance config with environment properties
-    if (_pinotEnvironmentProvider != null) {

Review comment:
       (Critical) the environment properties update is missing

##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
##########
@@ -396,6 +396,7 @@
     // but we keep this default for backward compatibility in case someone relies on this format
     // see Server or Broker class for correct prefix format you should use
     public static final String DEFAULT_METRICS_PREFIX = "pinot.controller.";
+    public static final String CONTROLLER_HELIX_INSTANCE_ID = "pinot.controller.helix.instance.id";

Review comment:
       Seems we already have different naming convention for different instance, but let's not add another one.. Suggest using `pinot.controller.instance.id` instead

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -139,12 +139,13 @@ public HelixBrokerStarter(PinotConfiguration brokerConf) throws Exception {
     if (brokerHost == null) {
       brokerHost = _brokerConf.getProperty(Helix.SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtils
           .getHostnameOrAddress() : NetUtils.getHostAddress();
+      brokerConf.setProperty(Broker.CONFIG_OF_BROKER_HOSTNAME, brokerHost);
     }
 
     _brokerId = _brokerConf.getProperty(Helix.Instance.INSTANCE_ID_KEY,
-        Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + _listenerConfigs.get(0).getPort());
+      Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + _listenerConfigs.get(0).getPort());

Review comment:
       Suggest also putting `port` as a member variable

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +498,76 @@ public IdealState apply(@Nullable IdealState idealState) {
       String tenant) {
     return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs, TagNameUtils.getBrokerTagForTenant(tenant)));
   }
+
+  /**
+   * Update host config in Helix if needed.
+   * The hostname and port cannot be null or empty;
+   * The port will be validated against integer.
+   * There is a callback lambda that can provide the tags if needed.
+   * For example () -> ImmutableList.of("Default_tenant")
+   * @param helixManager The Participant Manager
+   * @param instanceId the Helix instance id
+   * @param hostName the Hostname to update
+   * @param hostPort the host port to update
+   * @param getDefaultTags Something like () -> ImmutableList.of("Default_tenant") to provide default tags
+   */
+  public static void updateInstanceConfigIfNeeded(HelixManager helixManager, String instanceId, String hostName, String hostPort, Supplier<List<String>> getDefaultTags) {

Review comment:
       We can split this into multiple primitive operations and the individual starter can choose the primitives to use, e.g. `addDefaultInstanceTags()`, `updateHostnamePort()` etc

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -318,7 +319,22 @@ public void start()
         .registerMessageHandlerFactory(Message.MessageType.USER_DEFINE_MSG.toString(),
             new BrokerUserDefinedMessageHandlerFactory(_routingManager, queryQuotaManager));
     _participantHelixManager.connect();
-    addInstanceTagIfNeeded();
+
+    HelixHelper.updateInstanceConfigIfNeeded(

Review comment:
       Please set-up the code format per: https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup




-- 
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org