klsince commented on code in PR #12440:
URL: https://github.com/apache/pinot/pull/12440#discussion_r1548331232
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -287,4 +342,42 @@ private static PropertiesConfiguration
createPropertiesConfiguration(boolean set
return config;
}
+
+ /**
+ * Creates the IOFactory based on the provided kind.
+ *
+ * @param ioFactoryKind IOFactory kind
+ * @return IOFactory
+ */
+ private static IOFactory createPropertyIOFactory(PropertyIOFactoryKind
ioFactoryKind) {
+ switch (ioFactoryKind) {
+ case ConfigFileIOFactory:
+ return new ConfigFilePropertyIOFactory();
+ case SegmentMetadataIOFactory:
+ return new SegmentMetadataPropertyIOFactory();
+ default:
+ return new PropertiesConfiguration.DefaultIOFactory();
+ }
+ }
+
+ /**
+ * checks whether the segment metadata file first line is version header or
not.
+ * @param segmentMetadataFile segment metadata file
+ * @return boolean
+ * @throws IOException exception.
+ */
+ private static boolean checkHeaderExistsInSegmentMetadata(File
segmentMetadataFile) throws IOException {
+ Preconditions.checkNotNull(segmentMetadataFile,
Review Comment:
do we need to do null check? I don't feel so, but if there is need, let's
annotate `@Nullable File segmentMetadataFile`.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -287,4 +342,42 @@ private static PropertiesConfiguration
createPropertiesConfiguration(boolean set
return config;
}
+
+ /**
+ * Creates the IOFactory based on the provided kind.
+ *
+ * @param ioFactoryKind IOFactory kind
+ * @return IOFactory
+ */
+ private static IOFactory createPropertyIOFactory(PropertyIOFactoryKind
ioFactoryKind) {
+ switch (ioFactoryKind) {
+ case ConfigFileIOFactory:
+ return new ConfigFilePropertyIOFactory();
+ case SegmentMetadataIOFactory:
+ return new SegmentMetadataPropertyIOFactory();
+ default:
+ return new PropertiesConfiguration.DefaultIOFactory();
+ }
+ }
+
+ /**
+ * checks whether the segment metadata file first line is version header or
not.
+ * @param segmentMetadataFile segment metadata file
+ * @return boolean
+ * @throws IOException exception.
+ */
+ private static boolean checkHeaderExistsInSegmentMetadata(File
segmentMetadataFile) throws IOException {
+ Preconditions.checkNotNull(segmentMetadataFile,
+ "Segment Metadata file object can not be null for loading
configurations");
+
+ if (segmentMetadataFile.exists()) {
+ BufferedReader reader = new BufferedReader(new
FileReader(segmentMetadataFile));
Review Comment:
try this to close reader even upon any exception and save a few lines.
```
try(BufferredReader reader = ...) {
return StringUtils.startWith(...);
}
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -84,9 +90,10 @@ public static PropertiesConfiguration fromPath(String path)
* @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
setIOFactory, boolean setDefaultDelimiter,
Review Comment:
looks like those fromXXX(, ... PropertyIOFactoryKind ioFactoryKind) can be
`private` methods for now?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -128,6 +161,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)) {
+ String header = String.format("%s=%s", SEGMENT_VERSION_IDENTIFIER,
versionHeader);
+ propertiesConfiguration.setHeader(header);
+
+ // setIO appropriate factory
Review Comment:
the comment was not completed?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -287,4 +342,42 @@ private static PropertiesConfiguration
createPropertiesConfiguration(boolean set
return config;
}
+
+ /**
+ * Creates the IOFactory based on the provided kind.
+ *
+ * @param ioFactoryKind IOFactory kind
+ * @return IOFactory
+ */
+ private static IOFactory createPropertyIOFactory(PropertyIOFactoryKind
ioFactoryKind) {
+ switch (ioFactoryKind) {
+ case ConfigFileIOFactory:
+ return new ConfigFilePropertyIOFactory();
+ case SegmentMetadataIOFactory:
+ return new SegmentMetadataPropertyIOFactory();
+ default:
+ return new PropertiesConfiguration.DefaultIOFactory();
+ }
+ }
+
+ /**
+ * checks whether the segment metadata file first line is version header or
not.
+ * @param segmentMetadataFile segment metadata file
+ * @return boolean
+ * @throws IOException exception.
+ */
+ private static boolean checkHeaderExistsInSegmentMetadata(File
segmentMetadataFile) throws IOException {
+ Preconditions.checkNotNull(segmentMetadataFile,
+ "Segment Metadata file object can not be null for loading
configurations");
+
+ if (segmentMetadataFile.exists()) {
+ BufferedReader reader = new BufferedReader(new
FileReader(segmentMetadataFile));
+ String fileFirstLine = reader.readLine();
+ reader.close(); // close the reader
+
+ // check whether the segment has the version header or not
+ return StringUtils.contains(fileFirstLine, SEGMENT_VERSION_IDENTIFIER);
Review Comment:
startWith() to be more strict?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -54,7 +59,8 @@ private CommonsConfigurationUtils() {
*/
public static PropertiesConfiguration fromFile(File file)
throws ConfigurationException {
- return fromFile(file, false, true);
+ return fromFile(file, false, true,
+ PropertyIOFactoryKind.DefaultPropertyConfigurationIOFactory);
Review Comment:
It looks like we were using this one as the default
```
if (setIOFactory) {
config.setIOFactory(new ConfigFilePropertyReaderFactory()); // renamed
as ConfigFileIOFactory in this PR
```
If continue to use ConfigFileIOFactory as the default, we should be able to
avoid the two line changes in file PinotConfiguration.java
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -128,6 +161,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)) {
+ String header = String.format("%s=%s", SEGMENT_VERSION_IDENTIFIER,
versionHeader);
+ propertiesConfiguration.setHeader(header);
+
+ // setIO appropriate factory
+
propertiesConfiguration.setIOFactory(createPropertyIOFactory(PropertyIOFactoryKind.SegmentMetadataIOFactory));
+ }
+ saveToFile(propertiesConfiguration, file);
Review Comment:
the format was not right ^
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -100,24 +107,50 @@ public static PropertiesConfiguration fromPath(String
path, boolean setIOFactory
* @return a {@link PropertiesConfiguration} instance.
*/
public static PropertiesConfiguration fromInputStream(InputStream stream,
boolean setIOFactory,
- boolean setDefaultDelimiter)
+ boolean setDefaultDelimiter, PropertyIOFactoryKind ioFactoryKind)
throws ConfigurationException {
- PropertiesConfiguration config =
createPropertiesConfiguration(setIOFactory, setDefaultDelimiter);
+ PropertiesConfiguration config =
createPropertiesConfiguration(setIOFactory, setDefaultDelimiter, ioFactoryKind);
FileHandler fileHandler = new FileHandler(config);
fileHandler.load(stream);
return config;
}
+ /**
+ * Instantiate a Segment Metadata {@link PropertiesConfiguration} from a
{@link File}.
+ * @param file containing properties
+ * @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 segmentMetadataFromFile(File file,
boolean setIOFactory,
Review Comment:
call this `getSegmentMetadataFromFile`? to be a bit more consistent with
method name `saveSegmentMetadataToFile`
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/SegmentMetadataPropertyWriter.java:
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.Writer;
+import
org.apache.commons.configuration2.PropertiesConfiguration.PropertiesWriter;
+import org.apache.commons.configuration2.convert.ListDelimiterHandler;
+
+
+/**
+ * SegmentMetadataPropertyWriter extends the PropertiesWriter
+ * <p>
+ * Purpose: custom property writer for writing the segment metadata faster by
skipping the escaping of key.
+ */
+public class SegmentMetadataPropertyWriter extends PropertiesWriter {
+
+ public SegmentMetadataPropertyWriter(final Writer writer,
ListDelimiterHandler handler) {
+ super(writer, handler);
+ }
+
+ @Override
+ protected String escapeKey(final String key) {
+ // skip the escapeKey functionality,
+ return super.escapeKey(key);
Review Comment:
`return key` directly?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -100,24 +107,50 @@ public static PropertiesConfiguration fromPath(String
path, boolean setIOFactory
* @return a {@link PropertiesConfiguration} instance.
*/
public static PropertiesConfiguration fromInputStream(InputStream stream,
boolean setIOFactory,
- boolean setDefaultDelimiter)
+ boolean setDefaultDelimiter, PropertyIOFactoryKind ioFactoryKind)
throws ConfigurationException {
- PropertiesConfiguration config =
createPropertiesConfiguration(setIOFactory, setDefaultDelimiter);
+ PropertiesConfiguration config =
createPropertiesConfiguration(setIOFactory, setDefaultDelimiter, ioFactoryKind);
FileHandler fileHandler = new FileHandler(config);
fileHandler.load(stream);
return config;
}
+ /**
+ * Instantiate a Segment Metadata {@link PropertiesConfiguration} from a
{@link File}.
+ * @param file containing properties
+ * @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 segmentMetadataFromFile(File file,
boolean setIOFactory,
+ boolean setDefaultDelimiter)
+ throws ConfigurationException {
+ PropertyIOFactoryKind ioFactoryKind =
PropertyIOFactoryKind.DefaultPropertyConfigurationIOFactory;
+
+ try {
+ if (checkHeaderExistsInSegmentMetadata(file)) {
+ ioFactoryKind = PropertyIOFactoryKind.SegmentMetadataIOFactory;
+ }
+ } catch (IOException exception) {
+ throw new ConfigurationException(
+ String.format("Error occurred while reading segment metadata file %s
", file.getName()), exception);
+ }
+
+ return fromFile(file, setIOFactory, setDefaultDelimiter, ioFactoryKind);
+ }
+
/**
* Instantiate a {@link PropertiesConfiguration} from a {@link File}.
* @param file containing properties
* @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 fromFile(File file, boolean
setIOFactory, boolean setDefaultDelimiter)
+ public static PropertiesConfiguration fromFile(File file, boolean
setIOFactory,
+ boolean setDefaultDelimiter, PropertyIOFactoryKind ioFactoryKind)
throws ConfigurationException {
- PropertiesConfiguration config =
createPropertiesConfiguration(setIOFactory, setDefaultDelimiter);
+ Preconditions.checkNotNull(file, "File object can not be null for loading
configurations");
Review Comment:
do we need to do null check? I don't feel so, but if there is need, let's
annotate `@Nullable File file`.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -100,24 +107,50 @@ public static PropertiesConfiguration fromPath(String
path, boolean setIOFactory
* @return a {@link PropertiesConfiguration} instance.
*/
public static PropertiesConfiguration fromInputStream(InputStream stream,
boolean setIOFactory,
- boolean setDefaultDelimiter)
+ boolean setDefaultDelimiter, PropertyIOFactoryKind ioFactoryKind)
throws ConfigurationException {
- PropertiesConfiguration config =
createPropertiesConfiguration(setIOFactory, setDefaultDelimiter);
+ PropertiesConfiguration config =
createPropertiesConfiguration(setIOFactory, setDefaultDelimiter, ioFactoryKind);
FileHandler fileHandler = new FileHandler(config);
fileHandler.load(stream);
return config;
}
+ /**
+ * Instantiate a Segment Metadata {@link PropertiesConfiguration} from a
{@link File}.
+ * @param file containing properties
+ * @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 segmentMetadataFromFile(File file,
boolean setIOFactory,
+ boolean setDefaultDelimiter)
+ throws ConfigurationException {
+ PropertyIOFactoryKind ioFactoryKind =
PropertyIOFactoryKind.DefaultPropertyConfigurationIOFactory;
+
+ try {
+ if (checkHeaderExistsInSegmentMetadata(file)) {
+ ioFactoryKind = PropertyIOFactoryKind.SegmentMetadataIOFactory;
+ }
+ } catch (IOException exception) {
+ throw new ConfigurationException(
Review Comment:
looks like the `checkHeaderExistsInSegmentMetadata` doesn't throw this
exception? do we need to catch it here explicitly?
--
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
