[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9308: Deprecate instanceId Config For Broker/Minion Specific Configs

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

Jackie-Jiang commented on code in PR #9308:
URL: https://github.com/apache/pinot/pull/9308#discussion_r961339767


##########
pinot-minion/src/main/java/org/apache/pinot/minion/MinionConf.java:
##########
@@ -58,6 +59,9 @@ public int getPort() {
   }
 
   public String getInstanceId() {
+    if (StringUtils.isNotBlank(getProperty(CommonConstants.Minion.CONFIG_OF_MINION_ID))) {
+      return getProperty(CommonConstants.Minion.CONFIG_OF_MINION_ID);
+    }

Review Comment:
   (minor) maybe keeping the behavior consistent, and do null check for all of them
   ```suggestion
       String instanceId = getProperty(CommonConstants.Minion.CONFIG_OF_MINION_ID);
       return instanceId != null ? instanceId : getProperty(CommonConstants.Helix.Instance.INSTANCE_ID_KEY);
   ```



##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -133,14 +133,15 @@ public void init(PinotConfiguration brokerConf)
     _port = _listenerConfigs.get(0).getPort();
     _tlsPort = ListenerConfigUtil.findLastTlsPort(_listenerConfigs, -1);
 
-    _instanceId = _brokerConf.getProperty(Helix.Instance.INSTANCE_ID_KEY);
-    if (_instanceId != null) {
-      // NOTE: Force all instances to have the same prefix in order to derive the instance type based on the instance id
-      Preconditions.checkState(InstanceTypeUtils.isBroker(_instanceId), "Instance id must have prefix '%s', got '%s'",
-          Helix.PREFIX_OF_BROKER_INSTANCE, _instanceId);
-    } else {
+    _instanceId = _brokerConf.getProperty(Broker.CONFIG_OF_BROKER_ID);
+    if (_instanceId == null && _brokerConf.getProperty(Helix.Instance.INSTANCE_ID_KEY) != null) {
+      _instanceId = _brokerConf.getProperty(Helix.Instance.INSTANCE_ID_KEY);
+    } else if (_instanceId == null) {

Review Comment:
   ```suggestion
       _instanceId = _brokerConf.getProperty(Broker.CONFIG_OF_BROKER_ID);
       if (_instanceId == null) {
         _instanceId = _brokerConf.getProperty(Helix.Instance.INSTANCE_ID_KEY);
       }
       if (_instanceId == null) {
   ```



-- 
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: commits-unsubscribe@pinot.apache.org

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