Jackie-Jiang commented on a change in pull request #7064:
URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r660158809
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs,
TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the
getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For
example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () ->
ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>>
getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager,
instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
Review comment:
`instanceTags` won't be `null` here
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs,
TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the
getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For
example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () ->
ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>>
getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager,
instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null :
getDefaultTags.get();
Review comment:
`getDefaultTags` should never be `null`
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs,
TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the
getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For
example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () ->
ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>>
getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager,
instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null :
getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}",
instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager,
String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager,
InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of
HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+
helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()),
instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String
instanceId, String hostName, int hostPort) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager,
instanceId);
+ if (Strings.isNullOrEmpty(hostName)) {
+ LOGGER.info("host is empty, skip updating helix host.");
+ return false;
+ }
+ boolean updated = false;
+ // Update host and port if needed
+ if (!String.valueOf(hostPort).equals(instanceConfig.getPort())) {
+ instanceConfig.setPort(String.valueOf(hostPort));
+ updated = true;
+ }
+ if (!hostName.equals(instanceConfig.getHostName())) {
+ instanceConfig.setHostName(hostName);
+ updated = true;
+ }
+ updateInstanceConfig(helixManager, instanceConfig);
+ return updated;
+ }
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String
instanceId, String hostName, String hostPort) {
+ if (Strings.isNullOrEmpty(hostPort)) {
Review comment:
Same here. Also why do we log error here, while log info in the other
method?
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs,
TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the
getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For
example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () ->
ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>>
getDefaultTags) {
Review comment:
Pass in an `InstanceConfig` and a tag supplier, and return a boolean
denoting whether the config is changed. We don't want to read an instance
config and post it back for every update. We should only read the instance
config once, track whether it is updated, and only post it back once if there
are changes.
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs,
TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the
getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For
example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () ->
ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>>
getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager,
instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null :
getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}",
instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager,
String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager,
InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of
HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+
helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()),
instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String
instanceId, String hostName, int hostPort) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager,
instanceId);
+ if (Strings.isNullOrEmpty(hostName)) {
Review comment:
Can `hostName` be `null` or empty here? If so, should we skip updating
the port as well?
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs,
TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the
getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For
example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () ->
ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>>
getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager,
instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null :
getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}",
instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager,
String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager,
InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of
HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+
helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()),
instanceConfig),
+ "Failed to update instance config");
Review comment:
Log the instance id in the exception
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs,
TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the
getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For
example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () ->
ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>>
getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager,
instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null :
getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}",
instanceTags, instanceId);
Review comment:
`instanceTags` is always empty. We should log `defaultTags` instead
```suggestion
LOGGER.info("Updating instance: {} with default tags: {}",
instanceId, defaultTags);
```
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs,
TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the
getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For
example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () ->
ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>>
getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager,
instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null :
getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}",
instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager,
String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager,
InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of
HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+
helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()),
instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String
instanceId, String hostName, int hostPort) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager,
instanceId);
+ if (Strings.isNullOrEmpty(hostName)) {
Review comment:
We usually use `org.apache.commons.lang3.StringUtils` for string util
functions
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
##########
@@ -495,4 +499,104 @@ public IdealState apply(@Nullable IdealState idealState) {
String tenant) {
return new HashSet<>(getInstancesConfigsWithTag(instanceConfigs,
TagNameUtils.getBrokerTagForTenant(tenant)));
}
+
+ /**
+ * Add default tags to instance if the instance has no tags Note for the
getDefaultTags: it is a
+ * lambda so it may not be invoked if instance already has tags. * For
example () ->
+ * ImmutableList.of("Default_tenant")
+ *
+ * @param helixManager The helix manager for update
+ * @param instanceId the Helix instance Id
+ * @param getDefaultTags the default tags lambda. Something like () ->
ImmutableList.of("Default_tenant") to provide default tags
+ */
+ public static void addDefaultTags(
+ HelixManager helixManager, String instanceId, Supplier<List<String>>
getDefaultTags) {
+ InstanceConfig instanceConfig = getInstanceConfig(helixManager,
instanceId);
+ // Add default instance tags if not exist
+ List<String> instanceTags = instanceConfig.getTags();
+ if (instanceTags == null || instanceTags.size() == 0) {
+ List<String> defaultTags = getDefaultTags == null ? null :
getDefaultTags.get();
+ if (!CollectionUtils.isEmpty(defaultTags)) {
+ defaultTags.forEach(instanceConfig::addTag);
+ LOGGER.info("Updating instance tags {} for instance: {}",
instanceTags, instanceId);
+ updateInstanceConfig(helixManager, instanceConfig);
+ }
+ }
+ }
+
+ /**
+ * Returns the instance config for a specific instance ID
+ * @param helixManager the Helix manager
+ * @param instanceId the unique ID for instance
+ * @return An InstanceConfig that we can update
+ */
+ public static InstanceConfig getInstanceConfig(HelixManager helixManager,
String instanceId) {
+ HelixAdmin admin = helixManager.getClusterManagmentTool();
+ String clusterName = helixManager.getClusterName();
+ return admin.getInstanceConfig(clusterName, instanceId);
+ }
+
+ /**
+ * Update instance config into Helix properly
+ * @param helixManager the HelixManager for access
+ * @param instanceConfig the updated Helix Config
+ */
+ public static void updateInstanceConfig(HelixManager helixManager,
InstanceConfig instanceConfig) {
+ // NOTE: Use HelixDataAccessor.setProperty() instead of
HelixAdmin.setInstanceConfig() because the latter explicitly
+ // forbids instance host/port modification
+ HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+ Preconditions.checkState(
+
helixDataAccessor.setProperty(helixDataAccessor.keyBuilder().instanceConfig(instanceConfig.getId()),
instanceConfig),
+ "Failed to update instance config");
+ }
+
+ /**
+ * Update Helix Host and Name if the values are reasonable.
+ * @param helixManager the Helix Manager to get control
+ * @param instanceId the Helix instance Id
+ * @param hostName the Host name
+ * @param hostPort the Host port
+ * @return true if it is updated
+ */
+ public static boolean updateHostNamePort(HelixManager helixManager, String
instanceId, String hostName, int hostPort) {
Review comment:
Similar here, pass in the `InstanceConfig` instead of `helixManager` and
`instanceId`
--
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]
