Custom configuration property reader for segment metadata files #12440

abhioncbr · 2024-02-18T04:09:26Z

This is the first set of changes for the issue.

In this PR, we introduce a new custom property reader for the segment metadata files. This custom property reader is based on the commons-configuration2 custom property reader/writer(here is the link). We are introducing this custom property reader to avoid unescaping operation happen for the key in default implementation.

cc: @klsince @Jackie-Jiang @itschrispeck

codecov-commenter · 2024-02-18T04:47:23Z

Codecov Report

Attention: Patch coverage is 86.36364% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 62.16%. Comparing base (59551e4) to head (fbd5450).
Report is 522 commits behind head on master.

Files	Patch %	Lines
...pache/pinot/spi/env/CommonsConfigurationUtils.java	85.71%	4 Missing and 1 partial ⚠️
...rg/apache/pinot/spi/env/PropertyIOFactoryKind.java	70.00%	3 Missing ⚠️
.../apache/pinot/spi/env/VersionedPropertyReader.java	91.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #12440      +/-   ##
============================================
+ Coverage     61.75%   62.16%   +0.41%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2540     +104     
  Lines        133233   139455    +6222     
  Branches      20636    21554     +918     
============================================
+ Hits          82274    86697    +4423     
- Misses        44911    46265    +1354     
- Partials       6048     6493     +445

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`62.12% <86.36%> (+0.41%)`	⬆️
java-21	`62.05% <86.36%> (+0.42%)`	⬆️
skip-bytebuffers-false	`62.15% <86.36%> (+0.41%)`	⬆️
skip-bytebuffers-true	`62.02% <86.36%> (+34.29%)`	⬆️
temurin	`62.16% <86.36%> (+0.41%)`	⬆️
unittests	`62.16% <86.36%> (+0.41%)`	⬆️
unittests1	`46.75% <86.36%> (-0.14%)`	⬇️
unittests2	`27.72% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pinot-spi/src/main/java/org/apache/pinot/spi/env/SegmentMetadataPropertyReader.java

Jackie-Jiang

In order to keep backward compatibility, we will need to introduce versioning into the segment metadata file. Imagine we use the new property reader to read a old metadata file, it will fail to unescape the values, thus reading the wrong values.

We should introduce a new segment metadata version, and ensure there is no escaping in the new version, then we can use this new property reader for the new version. For the old version, we'll need to fall back to the old property reader.

pinot-spi/src/main/java/org/apache/pinot/spi/env/SegmentMetadataPropertyWriter.java

pinot-spi/src/main/java/org/apache/pinot/spi/env/SegmentMetadataPropertyReader.java

Jackie-Jiang

Can we add a test to ensure it can read the old format properly? We can put a old format file into the resource folder, and use the new reader to read it

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/V1Constants.java

abhioncbr · 2024-03-11T11:02:55Z

Can we add a test to ensure it can read the old format properly? We can put a old format file into the resource folder, and use the new reader to read it

Added the required tests.

abhioncbr · 2024-03-13T00:38:17Z

@Jackie-Jiang / @klsince, any further comments on the implementation side. I am going to start work on it's usage based on the cluster configuration or table configuration.

pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java

pinot-spi/src/main/java/org/apache/pinot/spi/env/SegmentMetadataPropertyWriter.java

pinot-spi/src/main/java/org/apache/pinot/spi/env/PropertyIOFactoryKind.java

pinot-spi/src/main/java/org/apache/pinot/spi/env/SegmentMetadataPropertyReader.java

pinot-spi/src/test/resources/segment-metadata-with-version-header.properties

abhioncbr · 2024-03-21T19:25:05Z

Based on the offline discussion, we decided to have pair of separate custom segment metadata reader/writer. We will decide to use custom reader/writer based on the version of the segment metadata.

pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java

pinot-spi/src/main/java/org/apache/pinot/spi/env/SegmentMetadataPropertyWriter.java

pinot-spi/src/test/resources/segment-metadata-with-version-header.properties

pinot-spi/src/test/resources/segment-metadata-without-version-header.properties

pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java

abhioncbr · 2024-04-10T01:28:04Z

@Jackie-Jiang / @klsince, please review it once more. Thanks

klsince

LGTM overall, w/ a few more questions and nits.

pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java

pinot-spi/src/test/java/org/apache/pinot/spi/env/SegmentMetadataPropertyConfigTest.java

… new custom IOFactory

…ta file.

…y changes.

…arator and added unit test case for it.

pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java

pinot-spi/src/main/java/org/apache/pinot/spi/env/VersionedPropertyReader.java

klsince · 2024-05-29T18:02:33Z

pinot-spi/src/main/java/org/apache/pinot/spi/env/VersionedPropertyReader.java

+            CommonsConfigurationUtils.VERSIONED_CONFIG_SEPARATOR));
+
+    String[] keyValue = line.split(getPropertySeparator());
+    Preconditions.checkArgument(keyValue.length == 2, "property content split should result in key and value");


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/CommonsConfigurationUtils.java

pinot-spi/src/main/java/org/apache/pinot/spi/env/VersionedPropertyReader.java

abhioncbr changed the title ~~Added custom configuration property reader for segment metadata.~~ Custom configuration property reader for segment metadata files Feb 18, 2024

abhioncbr mentioned this pull request Feb 20, 2024

commons-configuration2 performance issue during segment metadata load #12433

Open

abhioncbr force-pushed the 12433-segement-metadata-custom-reader branch from c429ffb to 3d68b96 Compare February 26, 2024 20:07

abhioncbr marked this pull request as ready for review February 26, 2024 20:08

klsince reviewed Feb 26, 2024

View reviewed changes

pinot-spi/src/main/java/org/apache/pinot/spi/env/SegmentMetadataPropertyReader.java Outdated Show resolved Hide resolved

Jackie-Jiang added the enhancement label Feb 26, 2024

Jackie-Jiang reviewed Feb 27, 2024

View reviewed changes

abhioncbr force-pushed the 12433-segement-metadata-custom-reader branch from a67b71a to 15601b7 Compare March 1, 2024 02:53

abhioncbr commented Mar 4, 2024

View reviewed changes

pinot-spi/src/main/java/org/apache/pinot/spi/env/SegmentMetadataPropertyWriter.java Outdated Show resolved Hide resolved

abhioncbr commented Mar 4, 2024

View reviewed changes

pinot-spi/src/main/java/org/apache/pinot/spi/env/SegmentMetadataPropertyReader.java Outdated Show resolved Hide resolved

Jackie-Jiang reviewed Mar 7, 2024

View reviewed changes

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/V1Constants.java Outdated Show resolved Hide resolved

abhioncbr force-pushed the 12433-segement-metadata-custom-reader branch from 157b780 to 86a2fff Compare March 11, 2024 11:02

klsince reviewed Mar 13, 2024

View reviewed changes

abhioncbr force-pushed the 12433-segement-metadata-custom-reader branch from c6ae01b to 1bc252a Compare March 15, 2024 06:05

klsince reviewed Mar 18, 2024

View reviewed changes

abhioncbr force-pushed the 12433-segement-metadata-custom-reader branch from d8104f6 to 2b0357a Compare March 21, 2024 19:01

abhioncbr force-pushed the 12433-segement-metadata-custom-reader branch from 2b0357a to 0f1dec9 Compare March 22, 2024 05:07

klsince reviewed Apr 2, 2024

View reviewed changes

abhioncbr force-pushed the 12433-segement-metadata-custom-reader branch from 0f1dec9 to 7afcc03 Compare April 5, 2024 00:13

Jackie-Jiang reviewed Apr 5, 2024

View reviewed changes

pinot-spi/src/test/resources/segment-metadata-with-version-header.properties Outdated Show resolved Hide resolved

Jackie-Jiang reviewed Apr 5, 2024

View reviewed changes

pinot-spi/src/test/resources/segment-metadata-without-version-header.properties Show resolved Hide resolved

Jackie-Jiang reviewed Apr 5, 2024

View reviewed changes

pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java Outdated Show resolved Hide resolved

abhioncbr force-pushed the 12433-segement-metadata-custom-reader branch from 4a6b4c3 to 18e562b Compare April 9, 2024 19:48

klsince reviewed Apr 17, 2024

View reviewed changes

abhioncbr force-pushed the 12433-segement-metadata-custom-reader branch from 18e562b to 5d89c55 Compare April 19, 2024 20:03

abhioncbr added 22 commits May 23, 2024 15:52

Added tests for sample segment metadata iwth and without header using…

4d8841f

… new custom IOFactory

remove some test because of header issue.

597a2be

Fixed failing test and removed unnecessary println.

b74d724

fixed linting issue

13af77f

fixed license check lint issue

ee008eb

another try for lint failure fix..

9ed37e7

Added tests for segment metadata with header along with sample metada…

11bfcc0

…ta file.

Fix rat check failure.

a2dbc8d

Fixed review commnets.

63eacb7

Updated the segment metadata properties config reader/writer.

1b56820

Added unit test cases for the newly added code and removed unnecessar…

79ed555

…y changes.

Changes as per the PR comments.

22265da

Fixed startsWith segment metadata version head check.

ffe343a

Changes as per the latest review comment.

c8da1c6

Chnages as per the PR comments.

d2e238b

more CommonsConfigurationUtils code refactoring and simpler UT's

9522b9e

Refactored code as per the PR comments.

3356357

Changes as per the PR comment.

75e09cf

renaming DefaultPropertyConfigurationIOFactory to DefaultIOFactory.

422c2b9

Some extra unit test cases for the changes.

048a7cf

Making sure that versioned configuration file gets written with = sep…

d76654b

…arator and added unit test case for it.

merged master

3bdbae3

abhioncbr force-pushed the 12433-segement-metadata-custom-reader branch from 0552e9e to 3bdbae3 Compare May 24, 2024 00:23

abhioncbr added 2 commits May 25, 2024 12:16

Updated as per the PR comment.

0cc919b

Fix failing UT.

f0ab9ad

klsince approved these changes May 28, 2024

View reviewed changes

Changes as per the PR comments.

05d728a

klsince reviewed May 29, 2024

View reviewed changes

abhioncbr added 2 commits May 30, 2024 17:27

Changes as per the PR comment.

651d6ae

minor fix.

fbd5450

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom configuration property reader for segment metadata files #12440

Custom configuration property reader for segment metadata files #12440

abhioncbr commented Feb 18, 2024 •

edited

codecov-commenter commented Feb 18, 2024 •

edited

Jackie-Jiang left a comment

Jackie-Jiang left a comment

abhioncbr commented Mar 11, 2024

abhioncbr commented Mar 13, 2024

abhioncbr commented Mar 21, 2024

abhioncbr commented Apr 10, 2024

klsince left a comment

klsince May 29, 2024

Custom configuration property reader for segment metadata files #12440

Are you sure you want to change the base?

Custom configuration property reader for segment metadata files #12440

Conversation

abhioncbr commented Feb 18, 2024 • edited

codecov-commenter commented Feb 18, 2024 • edited

Codecov Report

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

abhioncbr commented Mar 11, 2024

abhioncbr commented Mar 13, 2024

abhioncbr commented Mar 21, 2024

abhioncbr commented Apr 10, 2024

klsince left a comment

Choose a reason for hiding this comment

klsince May 29, 2024

Choose a reason for hiding this comment

abhioncbr commented Feb 18, 2024 •

edited

codecov-commenter commented Feb 18, 2024 •

edited