klsince commented on code in PR #12440: URL: https://github.com/apache/pinot/pull/12440#discussion_r1619284601
########## pinot-spi/src/main/java/org/apache/pinot/spi/env/VersionedPropertyReader.java: ########## @@ -0,0 +1,60 @@ +/** + * 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. ' = '. + String separator = getPropertySeparator(); + Preconditions.checkArgument(CommonsConfigurationUtils.VERSIONED_CONFIG_SEPARATOR.equals(separator), + String.format("versioned property configuration separator should be equal to '%s'", + CommonsConfigurationUtils.VERSIONED_CONFIG_SEPARATOR)); + + String[] keyValue = line.split(getPropertySeparator()); + Preconditions.checkArgument(keyValue.length == 2, "property content split should result in key and value"); Review Comment: This check might fail if the value happen to have the separator The `line.split()` does regex matching internally, so better use `StringUtils.split()`, and `StringUtils.split()` can take a `max` param to return just two parts to handle the issue said above. And since you have get `separator` above, you can save the calls of getPropertySeparator() on L48 and L52. ########## pinot-spi/src/main/java/org/apache/pinot/spi/env/VersionedPropertyReader.java: ########## @@ -0,0 +1,60 @@ +/** + * 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. ' = '. + String separator = getPropertySeparator(); + Preconditions.checkArgument(CommonsConfigurationUtils.VERSIONED_CONFIG_SEPARATOR.equals(separator), + String.format("versioned property configuration separator should be equal to '%s'", Review Comment: nit: "Versioned property configuration separator '%s' should be equal to '%s'" so we'd know what caused the error ########## 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); Review Comment: use StringUtils.split instead as commented below -- 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
