dongxiaoman commented on a change in pull request #7064: URL: https://github.com/apache/incubator-pinot/pull/7064#discussion_r659915596
########## File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java ########## @@ -53,9 +54,7 @@ public HelixBrokerStarter(PinotConfiguration brokerConf, String clusterName, Str private static PinotConfiguration applyBrokerConfigs(PinotConfiguration brokerConf, String clusterName, String zkServers, @Nullable String brokerHost) { brokerConf.setProperty(Helix.CONFIG_OF_CLUSTER_NAME, clusterName); brokerConf.setProperty(Helix.CONFIG_OF_ZOOKEEPR_SERVER, zkServers); - if (brokerHost == null) { - brokerConf.clearProperty(Broker.CONFIG_OF_BROKER_HOSTNAME); - } else { + if (!Strings.isNullOrEmpty(brokerHost)) { Review comment: Based on code reading, I don't see a use case that we need to clear the values. cc @Jackie-Jiang for double checking ########## File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java ########## @@ -495,4 +498,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 (defaultTags != null && !defaultTags.isEmpty()) { + defaultTags.forEach(instanceConfig::addTag); + LOGGER.info("Updating instance tags {} for instance: {}", instanceId, instanceTags); Review comment: Good catch. ########## 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: Changed to `.isEmpty()` ########## 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: All updated. Now the tracking logic is also in HelixHelper so the code for 4 host types are shared. ########## 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: Added. ########## 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: Changed ########## 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: Changed to use `org.apache.commons.lang3.StringUtils`; and `hostName` with `port` are both tested ########## 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: Right, all changed to `log.warn` which makes more sense ########## 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: Changed too. ########## 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: Added `NonNull` annotation and call directly. If no tags, we can call something like `ImmutableList::of` directly -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org