klsince commented on code in PR #12440:
URL: https://github.com/apache/pinot/pull/12440#discussion_r1617954347
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -276,20 +334,65 @@ 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, can be null.
+ * @return PropertiesConfiguration
+ */
+ private static PropertiesConfiguration createPropertiesConfiguration(boolean
setDefaultDelimiter,
+ @Nullable PropertyIOFactoryKind ioFactoryKind) {
PropertiesConfiguration config = new PropertiesConfiguration();
- // setting IO Reader Factory
- if (setIOFactory) {
- config.setIOFactory(new ConfigFilePropertyReaderFactory());
+ // setting IO Reader Factory of the configuration.
+ if (ioFactoryKind != null) {
+ config.setIOFactory(ioFactoryKind.getInstance());
}
- // setting DEFAULT_LIST_DELIMITER
+ // setting the DEFAULT_LIST_DELIMITER
if (setDefaultDelimiter) {
config.setListDelimiterHandler(new
LegacyListDelimiterHandler(DEFAULT_LIST_DELIMITER));
}
return config;
}
+
+ /**
+ * checks whether the configuration file first line is version header or not.
+ * @param file configuration file
+ * @return String
+ * @throws ConfigurationException exception.
+ */
+ private static String getConfigurationHeaderVersion(File file)
+ throws ConfigurationException {
+ String versionValue = DEFAULT_PROPERTIES_CONFIGURATION_HEADER_VERSION;
+ if (file.exists()) {
+ try (BufferedReader reader = new BufferedReader(new FileReader(file))) {
+ String fileFirstLine = reader.readLine();
+ // header version is written as a comment and start with '# '
+ String versionHeaderCommentPrefix = String.format("# %s",
VERSION_HEADER_IDENTIFIER);
+ // check whether the file has the version header or not
+ if (StringUtils.startsWith(fileFirstLine, versionHeaderCommentPrefix))
{
+ String[] headerKeyValue =
fileFirstLine.split(VERSIONED_CONFIG_SEPARATOR);
+ if (headerKeyValue.length == 2) {
+ versionValue = headerKeyValue[1];
+ }
+ }
+ } catch (IOException exception) {
+ throw new ConfigurationException(
+ String.format("Error occurred while reading configuration file %s
",
Review Comment:
nit: was the whitespace after `%s` left intentionally? otherwise can save
`String.format()` and simply `"Error ..." + file.getName()` for short, but not
a blocking comment any way.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/VersionedPropertyReader.java:
##########
@@ -0,0 +1,55 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.env;
+
+import com.google.common.base.Preconditions;
+import java.io.Reader;
+import
org.apache.commons.configuration2.PropertiesConfiguration.PropertiesReader;
+
+
+/**
+ * VersionedPropertyReader extends the PropertiesReader
+ * <p>
+ * Purpose: loads the segment metadata faster
+ * - by skipping the unescaping of key and
+ * - parsing the line by splitting based on first occurrence of separator
+ */
+class VersionedPropertyReader extends PropertiesReader {
+
+ public VersionedPropertyReader(Reader reader) {
+ super(reader);
+ }
+
+ @Override
+ protected void parseProperty(final String line) {
+ // skip the regex based parsing of the line content and splitting the
content based on first occurrence of separator
+ // getPropertySeparator(), in general returns the PropertiesConfiguration
`DEFAULT_SEPARATOR` value i.e. ' = '.
Review Comment:
how about adding a Preconditions.check on the return of
getPropertySeparator() to fail on unexpected separators explicitly
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -276,20 +334,65 @@ 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, can be null.
+ * @return PropertiesConfiguration
+ */
+ private static PropertiesConfiguration createPropertiesConfiguration(boolean
setDefaultDelimiter,
+ @Nullable PropertyIOFactoryKind ioFactoryKind) {
PropertiesConfiguration config = new PropertiesConfiguration();
- // setting IO Reader Factory
- if (setIOFactory) {
- config.setIOFactory(new ConfigFilePropertyReaderFactory());
+ // setting IO Reader Factory of the configuration.
+ if (ioFactoryKind != null) {
+ config.setIOFactory(ioFactoryKind.getInstance());
}
- // setting DEFAULT_LIST_DELIMITER
+ // setting the DEFAULT_LIST_DELIMITER
if (setDefaultDelimiter) {
config.setListDelimiterHandler(new
LegacyListDelimiterHandler(DEFAULT_LIST_DELIMITER));
}
return config;
}
+
+ /**
+ * checks whether the configuration file first line is version header or not.
+ * @param file configuration file
+ * @return String
+ * @throws ConfigurationException exception.
+ */
+ private static String getConfigurationHeaderVersion(File file)
+ throws ConfigurationException {
+ String versionValue = DEFAULT_PROPERTIES_CONFIGURATION_HEADER_VERSION;
+ if (file.exists()) {
+ try (BufferedReader reader = new BufferedReader(new FileReader(file))) {
+ String fileFirstLine = reader.readLine();
+ // header version is written as a comment and start with '# '
+ String versionHeaderCommentPrefix = String.format("# %s",
VERSION_HEADER_IDENTIFIER);
+ // check whether the file has the version header or not
+ if (StringUtils.startsWith(fileFirstLine, versionHeaderCommentPrefix))
{
+ String[] headerKeyValue =
fileFirstLine.split(VERSIONED_CONFIG_SEPARATOR);
+ if (headerKeyValue.length == 2) {
+ versionValue = headerKeyValue[1];
+ }
+ }
+ } catch (IOException exception) {
+ throw new ConfigurationException(
+ String.format("Error occurred while reading configuration file %s
",
+ file.getName()), exception);
+ }
+ }
+ return versionValue;
+ }
+
+ // Returns the version header string based on the version header value
provided.
+ // The return statement follow the pattern 'version = <value>'
+ static String getVersionHeaderString(@Nonnull String versionHeaderValue) {
Review Comment:
no need to annotate `@Nonnull` explicitly, as we use `@Nullable` explicitly
otherwise the params are nonnull
--
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
