Jackie-Jiang commented on code in PR #15648:
URL: https://github.com/apache/pinot/pull/15648#discussion_r2076495490
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -170,6 +178,34 @@ public static PropertiesConfiguration fromFile(@Nullable
File file,
return config;
}
+ /**
+ * Instantiate a {@link PropertiesConfiguration} from a {@link File}.
+ * @param file containing properties
+ * @param setDefaultDelimiter representing to set the default list delimiter.
+ * @param ioFactoryKind representing to set IOFactory. It can be null.
+ * @param failOnDuplicateKeys fail the loading when seeing duplicate keys.
+ * @return a {@link PropertiesConfiguration} instance.
+ */
+ public static PropertiesConfiguration fromFile(@Nullable File file, boolean
setDefaultDelimiter,
+ @Nullable PropertyIOFactoryKind ioFactoryKind, boolean
failOnDuplicateKeys)
Review Comment:
(minor) Can we move all handling into this method, and make the one on line
169 call this method with `failOnDeplicateKeys = false`?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -85,13 +87,18 @@ public static PropertiesConfiguration fromPath(String path)
public static PropertiesConfiguration fromPath(@Nullable String path,
boolean setDefaultDelimiter,
@Nullable PropertyIOFactoryKind ioFactoryKind)
Review Comment:
Can we also include `boolean failOnDuplicateKeys` as a parameter so that it
is clear to the caller that no duplicate keys are expected?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -170,6 +178,34 @@ public static PropertiesConfiguration fromFile(@Nullable
File file,
return config;
}
+ /**
+ * Instantiate a {@link PropertiesConfiguration} from a {@link File}.
+ * @param file containing properties
+ * @param setDefaultDelimiter representing to set the default list delimiter.
+ * @param ioFactoryKind representing to set IOFactory. It can be null.
+ * @param failOnDuplicateKeys fail the loading when seeing duplicate keys.
+ * @return a {@link PropertiesConfiguration} instance.
+ */
+ public static PropertiesConfiguration fromFile(@Nullable File file, boolean
setDefaultDelimiter,
+ @Nullable PropertyIOFactoryKind ioFactoryKind, boolean
failOnDuplicateKeys)
+ throws ConfigurationException {
+ if (failOnDuplicateKeys) {
+ PropertiesConfiguration noDuplicateKeysConfig =
createNoDuplicateKeysPropertiesConfiguration(ioFactoryKind);
+ // check if file exists, load the existing properties.
+ if (file != null && file.exists()) {
+ FileHandler fileHandler = new FileHandler(noDuplicateKeysConfig);
+ fileHandler.load(file);
+ }
+ PropertiesConfiguration finalConfig =
createPropertiesConfiguration(setDefaultDelimiter, ioFactoryKind);
Review Comment:
Any specific reason why we need to make a copy?
--
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
