klsince commented on code in PR #12440:
URL: https://github.com/apache/pinot/pull/12440#discussion_r1576650089
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -43,28 +46,16 @@
*/
public class CommonsConfigurationUtils {
private static final Character DEFAULT_LIST_DELIMITER = ',';
+ public static final String VERSION_HEADER_IDENTIFIER = "version";
Review Comment:
SEGMENT_METADATA_VERSION_HEADER_IDENTIFIER = "version" as we only use
versions to decide the reader/writer for the segment metadata, and leaving all
other users of this config util intact.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -271,20 +319,73 @@ public static String
recoverSpecialCharacterInPropertyValue(String value) {
return value.replace("\0\0", ",");
}
- private static PropertiesConfiguration createPropertiesConfiguration(boolean
setIOFactory,
- boolean setDefaultDelimiter) {
+ /**
+ * creates the instance of the {@link
org.apache.commons.configuration2.PropertiesConfiguration}
+ * with custom IO factory based on kind {@link
org.apache.commons.configuration2.PropertiesConfiguration.IOFactory}
+ * and legacy list delimiter {@link
org.apache.commons.configuration2.convert.LegacyListDelimiterHandler}
+ *
+ * @param setDefaultDelimiter sets the default list delimiter.
+ * @param ioFactoryKind IOFactory kind
+ * @return PropertiesConfiguration
+ */
+ private static PropertiesConfiguration createPropertiesConfiguration(boolean
setDefaultDelimiter,
+ PropertyIOFactoryKind ioFactoryKind) {
PropertiesConfiguration config = new PropertiesConfiguration();
- // setting IO Reader Factory
- if (setIOFactory) {
- config.setIOFactory(new ConfigFilePropertyReaderFactory());
- }
+ // setting IO Reader Factory of the configuration.
+ config.setIOFactory(createPropertyIOFactory(ioFactoryKind));
- // setting DEFAULT_LIST_DELIMITER
+ // setting the DEFAULT_LIST_DELIMITER
if (setDefaultDelimiter) {
config.setListDelimiterHandler(new
LegacyListDelimiterHandler(DEFAULT_LIST_DELIMITER));
}
return config;
}
+
+ /**
+ * Creates the IOFactory based on the provided kind.
+ * @param ioFactoryKind IOFactory kind
+ * @return IOFactory
+ */
+ private static IOFactory createPropertyIOFactory(PropertyIOFactoryKind
ioFactoryKind) {
Review Comment:
We can define a get() or getInstance() method in the Enum
`PropertyIOFactoryKind` so that we can remove this helper method
createPropertyIOFactory().
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -128,6 +156,18 @@ public static PropertiesConfiguration fromFile(File file,
boolean setIOFactory,
return config;
}
+ public static void saveSegmentMetadataToFile(PropertiesConfiguration
propertiesConfiguration, File file,
+ String versionHeader) {
+ if (StringUtils.isNotEmpty(versionHeader)) {
Review Comment:
I think you can do the check as part of the if-check condition.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -74,19 +65,19 @@ public static PropertiesConfiguration
fromInputStream(InputStream stream)
*/
public static PropertiesConfiguration fromPath(String path)
throws ConfigurationException {
- return fromPath(path, false, true);
+ return fromPath(path, true,
PropertyIOFactoryKind.DefaultPropertyConfigurationIOFactory);
}
/**
* Instantiate a {@link PropertiesConfiguration} from an {@link String}.
* @param path representing the path of file
- * @param setIOFactory representing to set the IOFactory or not
* @param setDefaultDelimiter representing to set the default list delimiter.
* @return a {@link PropertiesConfiguration} instance.
*/
- public static PropertiesConfiguration fromPath(String path, boolean
setIOFactory, boolean setDefaultDelimiter)
+ public static PropertiesConfiguration fromPath(String path, boolean
setDefaultDelimiter,
+ PropertyIOFactoryKind ioFactoryKind)
Review Comment:
how about pass in `@Nullable IOFactory ioFactory` to those util methods? so
that we decouple those util methods from the logic of deciding the type and
initializing the ioFactory object.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -43,28 +46,16 @@
*/
public class CommonsConfigurationUtils {
private static final Character DEFAULT_LIST_DELIMITER = ',';
+ public static final String VERSION_HEADER_IDENTIFIER = "version";
- private CommonsConfigurationUtils() {
- }
+ // usage: default header version of all configurations.
+ // if properties configuration doesn't contain header version, it will be
considered as 1
+ public static final String PROPERTIES_CONFIGURATION_HEADER_VERSION_1 = "1";
Review Comment:
I don't think we need "1" and can use `null` if there is no header from
segment metadata file.
The var name is not good as it encodes the value in the name. how about
`SEGMENT_METADATA_VERSION = 1` as default it's null.
```
public static final String PROPERTIES_CONFIGURATION_HEADER_VERSION_2 = "2";
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -271,20 +319,73 @@ public static String
recoverSpecialCharacterInPropertyValue(String value) {
return value.replace("\0\0", ",");
}
- private static PropertiesConfiguration createPropertiesConfiguration(boolean
setIOFactory,
- boolean setDefaultDelimiter) {
+ /**
+ * creates the instance of the {@link
org.apache.commons.configuration2.PropertiesConfiguration}
+ * with custom IO factory based on kind {@link
org.apache.commons.configuration2.PropertiesConfiguration.IOFactory}
+ * and legacy list delimiter {@link
org.apache.commons.configuration2.convert.LegacyListDelimiterHandler}
+ *
+ * @param setDefaultDelimiter sets the default list delimiter.
+ * @param ioFactoryKind IOFactory kind
+ * @return PropertiesConfiguration
+ */
+ private static PropertiesConfiguration createPropertiesConfiguration(boolean
setDefaultDelimiter,
+ PropertyIOFactoryKind ioFactoryKind) {
Review Comment:
allow to pass in null ioFactoryKind, so that we can keep previous logic
intact,
```
if (ioFactoryKind != null) {
config.setIOFactory(createPropertyIOFactory(ioFactoryKind));
}
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -128,13 +155,34 @@ public static PropertiesConfiguration fromFile(File file,
boolean setIOFactory,
return config;
}
+ /**
+ * save the segment metadata configuration content into the provided file
based on the version header.
+ * @param propertiesConfiguration a {@link PropertiesConfiguration} instance.
+ * @param file a {@link File} instance.
+ * @param versionHeader a {@link String} instance.
+ */
+ public static void saveSegmentMetadataToFile(PropertiesConfiguration
propertiesConfiguration, File file,
Review Comment:
mark `@Nullable versionHeader`
--
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
