klsince commented on code in PR #12440: URL: https://github.com/apache/pinot/pull/12440#discussion_r1608761946
########## pinot-spi/src/test/java/org/apache/pinot/spi/env/VersionedPropertyConfigTest.java: ########## @@ -0,0 +1,224 @@ +/** + * 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 java.io.File; +import java.io.IOException; +import java.util.List; +import java.util.Objects; +import org.apache.commons.configuration2.PropertiesConfiguration; +import org.apache.commons.configuration2.ex.ConfigurationException; +import org.apache.commons.io.FileUtils; +import org.apache.commons.lang3.RandomStringUtils; +import org.testng.annotations.AfterClass; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.assertNull; + + +public class VersionedPropertyConfigTest { + + private static final String COLON_SEPARATOR = ":"; + private static final File TEMP_DIR = new File(FileUtils.getTempDirectory(), "VersionedPropertyConfigTest"); + private static final File CONFIG_FILE = new File(TEMP_DIR, "config"); + private static final String[] TEST_PROPERTY_KEY = { "test1", "test2_key", "test3_key_", + "test4_key_1234", "test-1", "test.1" }; + private static final String[] TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR = { "test:1", "test2=key", Review Comment: curious how keys with `=` like `test2=key` get handled properly? because the two white spaces in the separator constant defined in `CommonsConfigurationUtils.java` above? ########## pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java: ########## @@ -42,29 +44,20 @@ * Provide utility functions to manipulate Apache Commons {@link Configuration} instances. */ public class CommonsConfigurationUtils { + + private static final String VERSIONED_CONFIG_SEPARATOR = " = "; Review Comment: is the white space required? ########## pinot-spi/src/test/java/org/apache/pinot/spi/env/CommonsConfigurationUtilsTest.java: ########## @@ -50,6 +53,33 @@ public void tearDown() FileUtils.deleteDirectory(TEMP_DIR); } + @Test + public void testSegmentMetadataFromFile() { + // load the existing config and check the properties config instance + try { + PropertiesConfiguration config = CommonsConfigurationUtils + .getSegmentMetadataFromFile(SEGMENT_METADATA_CONFIG_FILE, true); + assertNotNull(config); + + config.setProperty("testKey", "testValue"); + + // add the segment version header to the file and read it again + CommonsConfigurationUtils.saveSegmentMetadataToFile(config, CONFIG_FILE, + CommonsConfigurationUtils.PROPERTIES_CONFIGURATION_HEADER_VERSION_2); + + // reading the property with header. + config = CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true); + assertNotNull(config); + assertEquals(config.getHeader(), "# version=2"); + } catch (Exception ex) { + Assert.fail("should not throw ConfigurationException exception with valid file"); + } + + // load the non-existing file and expect the exception + Assert.expectThrows(NullPointerException.class, + () -> CommonsConfigurationUtils.getSegmentMetadataFromFile(null, true)); Review Comment: > we don't need the non-existing file test. is this because we do `fileHandler.setFile(file);` if file doesn't exist? not introduced by this PR, but looks like you added those changes earlier on, so in case you know: 1. for `fromFile()`, why we need to do `fileHandler.setFile(file)` when file doesn't exit? 2. if that's needed, for `fromPath()` method, should we do `fileHandler.setPath(path)` if path doesn't exist as well? ########## pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java: ########## @@ -271,20 +328,59 @@ 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("="); Review Comment: Use the separator constant? -- 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
