You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2023/05/24 00:24:56 UTC
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10771: Enable case insensitivity by default
Jackie-Jiang commented on code in PR #10771:
URL: https://github.com/apache/pinot/pull/10771#discussion_r1203194721
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -314,7 +314,7 @@ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, Notific
Map<String, String> configs = _helixAdmin.getConfig(helixConfigScope,
Arrays.asList(Helix.ENABLE_CASE_INSENSITIVE_KEY, Helix.DEPRECATED_ENABLE_CASE_INSENSITIVE_KEY));
boolean caseInsensitive =
- Boolean.parseBoolean(configs.get(Helix.ENABLE_CASE_INSENSITIVE_KEY)) || Boolean.parseBoolean(
+ Boolean.parseBoolean(configs.get(Helix.ENABLE_CASE_INSENSITIVE_KEY)) && Boolean.parseBoolean(
Review Comment:
This is incorrect
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/HelixSetupUtils.java:
##########
@@ -79,7 +79,8 @@ private static void setupHelixClusterIfNeeded(String helixClusterName, String zk
new HelixConfigScopeBuilder(ConfigScopeProperty.CLUSTER).forCluster(helixClusterName).build();
Map<String, String> configMap = new HashMap<>();
configMap.put(ZKHelixManager.ALLOW_PARTICIPANT_AUTO_JOIN, Boolean.toString(true));
- configMap.put(ENABLE_CASE_INSENSITIVE_KEY, Boolean.toString(false));
+ configMap.put(ENABLE_CASE_INSENSITIVE_KEY, Boolean.toString(DEFAULT_ENABLE_CASE_INSENSITIVE));
+ configMap.put(DEPRECATED_ENABLE_CASE_INSENSITIVE_KEY, Boolean.toString(DEFAULT_ENABLE_CASE_INSENSITIVE));
Review Comment:
Let's not put the deprecated key
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -270,8 +270,9 @@ public void start()
// Initialize FunctionRegistry before starting the broker request handler
FunctionRegistry.init();
boolean caseInsensitive =
- _brokerConf.getProperty(Helix.ENABLE_CASE_INSENSITIVE_KEY, false) || _brokerConf.getProperty(
- Helix.DEPRECATED_ENABLE_CASE_INSENSITIVE_KEY, false);
+ _brokerConf.getProperty(Helix.ENABLE_CASE_INSENSITIVE_KEY, Helix.DEFAULT_ENABLE_CASE_INSENSITIVE)
Review Comment:
Not introduced in this PR, but the correct way to handle it should be:
1. Check if the key is explicitly configured
2. If so, use the value
3. If not, fall back to the deprecated key
Currently, if the official key is configured as `true`, but the deprecated key is configured as `false`, the overall result is `false` which is unexpected
--
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