Jackie-Jiang commented on code in PR #11985:
URL: https://github.com/apache/pinot/pull/11985#discussion_r1427605236
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -328,8 +265,13 @@ private static PropertiesConfiguration
createPropertiesConfiguration(boolean set
// setting DEFAULT_LIST_DELIMITER
if (setDefaultDelimiter) {
- CommonsConfigurationUtils.setDefaultListDelimiterHandler(config);
+ CommonsConfigurationUtils.setListDelimiterHandler(config);
Review Comment:
(minor)
```suggestion
setListDelimiterHandler(config);
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -288,6 +195,7 @@ public static <T> T convert(Object value, Class<T>
returnType) {
* - Escaping comma with backslash doesn't work when comma is preceded by a
backslash
*/
public static String replaceSpecialCharacterInPropertyValue(String value) {
+ value = StringEscapeUtils.escapeJava(value);
Review Comment:
Can you give an example of what the property will look like in v1 vs v2?
Will v1 automatically escape/unescape the value?
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/utils/SegmentMetadataUtils.java:
##########
@@ -32,22 +33,24 @@ public class SegmentMetadataUtils {
private SegmentMetadataUtils() {
}
- public static PropertiesConfiguration getPropertiesConfiguration(File
indexDir) {
+ public static PropertiesConfiguration getPropertiesConfiguration(File
indexDir)
+ throws ConfigurationException {
File metadataFile = SegmentDirectoryPaths.findMetadataFile(indexDir);
Preconditions.checkNotNull(metadataFile, "Cannot find segment metadata
file under directory: %s", indexDir);
return CommonsConfigurationUtils.fromFile(metadataFile);
}
- public static PropertiesConfiguration
getPropertiesConfiguration(SegmentMetadata segmentMetadata) {
+ public static PropertiesConfiguration
getPropertiesConfiguration(SegmentMetadata segmentMetadata)
+ throws ConfigurationException {
File indexDir = segmentMetadata.getIndexDir();
Preconditions.checkState(indexDir != null, "Cannot get
PropertiesConfiguration from in-memory segment: %s",
segmentMetadata.getName());
return getPropertiesConfiguration(indexDir);
}
- public static void savePropertiesConfiguration(PropertiesConfiguration
propertiesConfiguration) {
- File metadataFile = propertiesConfiguration.getFile();
- Preconditions.checkState(metadataFile != null, "Cannot save
PropertiesConfiguration not loaded from file");
+ public static void savePropertiesConfiguration(PropertiesConfiguration
propertiesConfiguration, File indexDir) {
+ File metadataFile = SegmentDirectoryPaths.findMetadataFile(indexDir);
+ Preconditions.checkNotNull(metadataFile, "Cannot find segment metadata
file under directory: %s", indexDir);
Review Comment:
(minor) We probably want to throw `IllegalStateException` instead of
`NullPointerException`
```suggestion
Preconditions.checkState(metadataFile != null, "Cannot find segment
metadata file under directory: %s", indexDir);
```
--
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
