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: [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]
