You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "abhioncbr (via GitHub)" <gi...@apache.org> on 2023/11/10 02:34:29 UTC
[PR] Changes for migration to commons-configuration2 [pinot]
abhioncbr opened a new pull request, #11985:
URL: https://github.com/apache/pinot/pull/11985
As per the [issue](https://github.com/apache/pinot/issues/11085) and in continuation of the previous work https://github.com/apache/pinot/pull/11792, https://github.com/apache/pinot/pull/11868, https://github.com/apache/pinot/pull/11916, This PR upgrade the `Metadata` to `commons-configuartion2`.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Changes for migration to commons-configuration2 [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11985:
URL: https://github.com/apache/pinot/pull/11985#discussion_r1417996293
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -47,100 +47,77 @@ public class CommonsConfigurationUtils {
private CommonsConfigurationUtils() {
}
- public static void setListDelimiterHandler(PropertiesConfiguration configuration, Character delimiter) {
- configuration.setListDelimiterHandler(new DefaultListDelimiterHandler(delimiter));
+ /**
+ * Instantiate a {@link PropertiesConfiguration} from a {@link File}.
+ * @param file containing properties
+ * @return a {@link PropertiesConfiguration} instance. Empty if file does not exist.
+ */
+ public static PropertiesConfiguration fromFile(File file) {
+ try {
+ return fromFile(file, false, false);
+ } catch (ConfigurationException e) {
Review Comment:
Should we keep the checked exception to have consistent behavior for all methods?
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java:
##########
@@ -1898,15 +1899,19 @@ private File buildTestSegment(final TableConfig tableConfig, final Schema schema
private static void removeMinMaxValuesFromMetadataFile(File indexDir) {
PropertiesConfiguration configuration = SegmentMetadataUtils.getPropertiesConfiguration(indexDir);
Iterator<String> keys = configuration.getKeys();
+ LinkedList<String> keysToClear = new LinkedList<>();
Review Comment:
(minor)
```suggestion
List<String> keysToClear = new ArrayList<>();
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -317,19 +242,57 @@ public static String recoverSpecialCharacterInPropertyValue(String value) {
return value.replace("\0\0", ",");
}
+ /**
Review Comment:
Do we need this? After setting the list delimiter, will `getList()` automatically treat `,` as list delimiter?
If not, this is high risk change because other developer might not know they cannot use `getList()` to retrieve the list values
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Changes for migration to commons-configuration2 [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11985:
URL: https://github.com/apache/pinot/pull/11985#discussion_r1427605236
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -328,8 +265,13 @@ private static PropertiesConfiguration createPropertiesConfiguration(boolean set
// setting DEFAULT_LIST_DELIMITER
if (setDefaultDelimiter) {
- CommonsConfigurationUtils.setDefaultListDelimiterHandler(config);
+ CommonsConfigurationUtils.setListDelimiterHandler(config);
Review Comment:
(minor)
```suggestion
setListDelimiterHandler(config);
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -288,6 +195,7 @@ public static <T> T convert(Object value, Class<T> returnType) {
* - Escaping comma with backslash doesn't work when comma is preceded by a backslash
*/
public static String replaceSpecialCharacterInPropertyValue(String value) {
+ value = StringEscapeUtils.escapeJava(value);
Review Comment:
Can you give an example of what the property will look like in v1 vs v2? Will v1 automatically escape/unescape the value?
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/utils/SegmentMetadataUtils.java:
##########
@@ -32,22 +33,24 @@ public class SegmentMetadataUtils {
private SegmentMetadataUtils() {
}
- public static PropertiesConfiguration getPropertiesConfiguration(File indexDir) {
+ public static PropertiesConfiguration getPropertiesConfiguration(File indexDir)
+ throws ConfigurationException {
File metadataFile = SegmentDirectoryPaths.findMetadataFile(indexDir);
Preconditions.checkNotNull(metadataFile, "Cannot find segment metadata file under directory: %s", indexDir);
return CommonsConfigurationUtils.fromFile(metadataFile);
}
- public static PropertiesConfiguration getPropertiesConfiguration(SegmentMetadata segmentMetadata) {
+ public static PropertiesConfiguration getPropertiesConfiguration(SegmentMetadata segmentMetadata)
+ throws ConfigurationException {
File indexDir = segmentMetadata.getIndexDir();
Preconditions.checkState(indexDir != null, "Cannot get PropertiesConfiguration from in-memory segment: %s",
segmentMetadata.getName());
return getPropertiesConfiguration(indexDir);
}
- public static void savePropertiesConfiguration(PropertiesConfiguration propertiesConfiguration) {
- File metadataFile = propertiesConfiguration.getFile();
- Preconditions.checkState(metadataFile != null, "Cannot save PropertiesConfiguration not loaded from file");
+ public static void savePropertiesConfiguration(PropertiesConfiguration propertiesConfiguration, File indexDir) {
+ File metadataFile = SegmentDirectoryPaths.findMetadataFile(indexDir);
+ Preconditions.checkNotNull(metadataFile, "Cannot find segment metadata file under directory: %s", indexDir);
Review Comment:
(minor) We probably want to throw `IllegalStateException` instead of `NullPointerException`
```suggestion
Preconditions.checkState(metadataFile != null, "Cannot find segment metadata file under directory: %s", indexDir);
```
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Changes for migration to commons-configuration2 [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11985:
URL: https://github.com/apache/pinot/pull/11985#issuecomment-1820215175
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `21 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`972b555`)](https://app.codecov.io/gh/apache/pinot/commit/972b555cc5609a88002ac2c4501ece247b51cebe?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.44% compared to head [(`70122e8`)](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 46.81%.
> Report is 46 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...pache/pinot/spi/env/CommonsConfigurationUtils.java](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZW52L0NvbW1vbnNDb25maWd1cmF0aW9uVXRpbHMuamF2YQ==) | 73.33% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ocal/startree/v2/builder/MultipleTreesBuilder.java](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9idWlsZGVyL011bHRpcGxlVHJlZXNCdWlsZGVyLmphdmE=) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ment/creator/impl/SegmentColumnarIndexCreator.java](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50Q29sdW1uYXJJbmRleENyZWF0b3IuamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ndex/converter/SegmentV1V2ToV3FormatConverter.java](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbnZlcnRlci9TZWdtZW50VjFWMlRvVjNGb3JtYXRDb252ZXJ0ZXIuamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../columnminmaxvalue/ColumnMinMaxValueGenerator.java](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9jb2x1bW5taW5tYXh2YWx1ZS9Db2x1bW5NaW5NYXhWYWx1ZUdlbmVyYXRvci5qYXZh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...loader/defaultcolumn/BaseDefaultColumnHandler.java](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9kZWZhdWx0Y29sdW1uL0Jhc2VEZWZhdWx0Q29sdW1uSGFuZGxlci5qYXZh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../local/segment/store/SingleFileIndexDirectory.java](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3N0b3JlL1NpbmdsZUZpbGVJbmRleERpcmVjdG9yeS5qYXZh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...t/segment/local/startree/StarTreeBuilderUtils.java](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS9TdGFyVHJlZUJ1aWxkZXJVdGlscy5qYXZh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...local/startree/v2/store/StarTreeIndexMapUtils.java](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9zdG9yZS9TdGFyVHJlZUluZGV4TWFwVXRpbHMuamF2YQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #11985 +/- ##
=============================================
- Coverage 61.44% 46.81% -14.64%
+ Complexity 1141 944 -197
=============================================
Files 2385 1787 -598
Lines 129189 93897 -35292
Branches 19998 15178 -4820
=============================================
- Hits 79381 43956 -35425
- Misses 44061 46830 +2769
+ Partials 5747 3111 -2636
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11985/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/11985/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/11985/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/11985/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/11985/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/11985/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/11985/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.81% <55.31%> (-14.50%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/11985/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/11985/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.81% <55.31%> (-14.46%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11985/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.81% <55.31%> (-14.64%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11985/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.81% <55.31%> (-14.63%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11985/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.81% <55.31%> (+0.11%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/11985/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/11985?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Changes for migration to commons-configuration2 [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #11985:
URL: https://github.com/apache/pinot/pull/11985#discussion_r1426110737
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -317,19 +242,57 @@ public static String recoverSpecialCharacterInPropertyValue(String value) {
return value.replace("\0\0", ",");
}
+ /**
Review Comment:
Instead of using [DefaultListDelimiterHandler](https://commons.apache.org/proper/commons-configuration/apidocs/org/apache/commons/configuration2/convert/DefaultListDelimiterHandler.html), [LegacyListDelimiterHandler](https://commons.apache.org/proper/commons-configuration/apidocs/org/apache/commons/configuration2/convert/LegacyListDelimiterHandler.html) follows the commons-configuration1 implementation. Reverting the changes related to spliting the string values.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Changes for migration to commons-configuration2 [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #11985:
URL: https://github.com/apache/pinot/pull/11985#discussion_r1414394951
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -317,19 +242,57 @@ public static String recoverSpecialCharacterInPropertyValue(String value) {
return value.replace("\0\0", ",");
}
+ /**
Review Comment:
Moved the function [getStringListFromSegmentProperties](https://github.com/apache/pinot/blob/master/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/LoaderUtils.java#L70C30-L70C64) to here, since commons-configuration2 saves list-based data individually, for exampel
```bash
segment.dimension.column.names = bytesDimSV1
segment.dimension.column.names = generationNumber
segment.dimension.column.names = intDimMV1
segment.dimension.column.names = intDimMV2
segment.dimension.column.names = longDimSV1
segment.dimension.column.names = longDimSV2
segment.dimension.column.names = mapDim1__KEYS
segment.dimension.column.names = mapDim1__VALUES
segment.dimension.column.names = mapDim2json
segment.dimension.column.names = stringDimMV1
segment.dimension.column.names = stringDimMV2
segment.dimension.column.names = stringDimSV1
segment.dimension.column.names = stringDimSV2
segment.dimension.column.names = textDim1
segment.metric.column.names = doubleMetric1
segment.metric.column.names = floatMetric1
segment.metric.column.names = intMetric1
segment.metric.column.names = longMetric1
```
to make this migration compatible, enhanced the function to split the values in-case if it has comma `,`
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Changes for migration to commons-configuration2 [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #11985:
URL: https://github.com/apache/pinot/pull/11985#discussion_r1424747131
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -317,19 +242,57 @@ public static String recoverSpecialCharacterInPropertyValue(String value) {
return value.replace("\0\0", ",");
}
+ /**
Review Comment:
I'll try to explain it to you.
In commons-configuration1, we were setting Comma `,` as ListDelimiter. The metadata files written with this property result in writing the list values in one line separated by a comma. For string values having the comma, we are[ replacing it with](https://github.com/apache/pinot/blob/master/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java#L300C31-L300C37) `\0\0`
The same setup with commons-configuration2 is working differently. Commons-configuration2 internally changed the implementation of escaping/unescaping the values. I have witnessed the issue of handling string values with leading or trailing whitespace.
I got the upgrade working with backward compatibility by not setting Comma `,` as a list delimiter. Since there is no delimiter set, commons-configuration2 saves the list property values individually in a separate line, for example like this
```bash
segment.dimension.column.names = bytesDimSV1
segment.dimension.column.names = generationNumber
segment.dimension.column.names = intDimMV1
segment.dimension.column.names = intDimMV2
segment.dimension.column.names = longDimSV1
```
And, to make it backward compatible, we are splitting the list values based on comma `,`.
for escaping and unescaping we are now using `StringEscapeUtils` functionality. Please, let me know if this looks correct to you. Thanks
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Changes for migration to commons-configuration2 [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #11985:
URL: https://github.com/apache/pinot/pull/11985#discussion_r1426126772
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -288,6 +195,7 @@ public static <T> T convert(Object value, Class<T> returnType) {
* - Escaping comma with backslash doesn't work when comma is preceded by a backslash
*/
public static String replaceSpecialCharacterInPropertyValue(String value) {
+ value = StringEscapeUtils.escapeJava(value);
Review Comment:
This is needed in commons-configuration2, as it does not support string escape/unescape like commons-configuration1.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Changes for migration to commons-configuration2 [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #11985:
URL: https://github.com/apache/pinot/pull/11985#discussion_r1415456015
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java:
##########
@@ -1898,15 +1899,19 @@ private File buildTestSegment(final TableConfig tableConfig, final Schema schema
private static void removeMinMaxValuesFromMetadataFile(File indexDir) {
PropertiesConfiguration configuration = SegmentMetadataUtils.getPropertiesConfiguration(indexDir);
Iterator<String> keys = configuration.getKeys();
+ LinkedList<String> keysToClear = new LinkedList<>();
while (keys.hasNext()) {
String key = keys.next();
if (key.endsWith(V1Constants.MetadataKeys.Column.MIN_VALUE) || key.endsWith(
V1Constants.MetadataKeys.Column.MAX_VALUE) || key.endsWith(
V1Constants.MetadataKeys.Column.MIN_MAX_VALUE_INVALID)) {
- configuration.clearProperty(key);
+ keysToClear.add(key);
Review Comment:
`configuration.clearProperty(key);` results in a concurrent modification exception and hence a workaround to clear the keys by first getting the list of keys to clear and later clearing it.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Changes for migration to commons-configuration2 [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11985:
URL: https://github.com/apache/pinot/pull/11985#discussion_r1423319027
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -47,100 +47,69 @@ public class CommonsConfigurationUtils {
private CommonsConfigurationUtils() {
}
- public static void setListDelimiterHandler(PropertiesConfiguration configuration, Character delimiter) {
- configuration.setListDelimiterHandler(new DefaultListDelimiterHandler(delimiter));
+ /**
+ * Instantiate a {@link PropertiesConfiguration} from a {@link File}.
+ * @param file containing properties
+ * @return a {@link PropertiesConfiguration} instance. Empty if file does not exist.
+ */
+ public static PropertiesConfiguration fromFile(File file) throws ConfigurationException {
Review Comment:
(minor) Please apply `Pinot Style`, same for other changes
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -317,19 +242,57 @@ public static String recoverSpecialCharacterInPropertyValue(String value) {
return value.replace("\0\0", ",");
}
+ /**
Review Comment:
Sorry I still don't fully follow. Is there behavior change when calling `getList()` on comma separated list? Why do we need to explicitly split the value here?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -288,6 +195,7 @@ public static <T> T convert(Object value, Class<T> returnType) {
* - Escaping comma with backslash doesn't work when comma is preceded by a backslash
*/
public static String replaceSpecialCharacterInPropertyValue(String value) {
+ value = StringEscapeUtils.escapeJava(value);
Review Comment:
Is this a bugfix?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Changes for migration to commons-configuration2 [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #11985:
URL: https://github.com/apache/pinot/pull/11985
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Changes for migration to commons-configuration2 [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #11985:
URL: https://github.com/apache/pinot/pull/11985#issuecomment-1851143998
Please review once more @Jackie-Jiang. Thanks
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Changes for migration to commons-configuration2 [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #11985:
URL: https://github.com/apache/pinot/pull/11985#discussion_r1419847374
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -317,19 +242,57 @@ public static String recoverSpecialCharacterInPropertyValue(String value) {
return value.replace("\0\0", ",");
}
+ /**
Review Comment:
They can use `getList()` without any issue with this change. It's just setting COMMA `,` which results in weird escaping issues and results in failure for values with leading or trailing spaces or whitespace.
After certain iterations, I got this working with backward compatibility.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Changes for migration to commons-configuration2 [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #11985:
URL: https://github.com/apache/pinot/pull/11985#discussion_r1420619096
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -47,100 +47,77 @@ public class CommonsConfigurationUtils {
private CommonsConfigurationUtils() {
}
- public static void setListDelimiterHandler(PropertiesConfiguration configuration, Character delimiter) {
- configuration.setListDelimiterHandler(new DefaultListDelimiterHandler(delimiter));
+ /**
+ * Instantiate a {@link PropertiesConfiguration} from a {@link File}.
+ * @param file containing properties
+ * @return a {@link PropertiesConfiguration} instance. Empty if file does not exist.
+ */
+ public static PropertiesConfiguration fromFile(File file) {
+ try {
+ return fromFile(file, false, false);
+ } catch (ConfigurationException e) {
Review Comment:
Sure, previously, these functions were throwing the Runtime exception. It requires handling several places.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Changes for migration to commons-configuration2 [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #11985:
URL: https://github.com/apache/pinot/pull/11985#issuecomment-1841436894
@Jackie-Jiang / @xiangfu0 / @walterddr please review. Thanks
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Changes for migration to commons-configuration2 [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #11985:
URL: https://github.com/apache/pinot/pull/11985#discussion_r1424747131
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -317,19 +242,57 @@ public static String recoverSpecialCharacterInPropertyValue(String value) {
return value.replace("\0\0", ",");
}
+ /**
Review Comment:
I'll try to explain it to you.
In commons-configuration1, we were setting Comma `,` as ListDelimiter. The metadata files written with this property result in writing the list values in one line separated by a comma. For string values having the comma, we are[ replacing it with](https://github.com/apache/pinot/blob/master/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java#L300C31-L300C37) `\0\0` to distinguish it from the list.
The same setup with commons-configuration2 is working differently. Commons-configuration2 internally changed the implementation of escaping/unescaping the values. I have witnessed the issue of handling string values with leading or trailing whitespace.
I got the upgrade working with backward compatibility by not setting Comma `,` as a list delimiter. Since there is no delimiter set, commons-configuration2 saves the list property values individually in a separate line, for example like this
```bash
segment.dimension.column.names = bytesDimSV1
segment.dimension.column.names = generationNumber
segment.dimension.column.names = intDimMV1
segment.dimension.column.names = intDimMV2
segment.dimension.column.names = longDimSV1
```
And, to make it backward compatible, we are splitting the list values based on comma `,`.
for escaping and unescaping we are now using `StringEscapeUtils` functionality. Please, let me know if this looks correct to you. Thanks
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org