-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes for migration to commons-configuration2 #11985
Changes for migration to commons-configuration2 #11985
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #11985 +/- ##
============================================
- Coverage 61.64% 61.64% -0.01%
- Complexity 1152 1153 +1
============================================
Files 2407 2407
Lines 130923 130907 -16
Branches 20223 20222 -1
============================================
- Hits 80705 80693 -12
+ Misses 44338 44325 -13
- Partials 5880 5889 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
cb96abd
to
49a2203
Compare
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java
Show resolved
Hide resolved
@Jackie-Jiang / @xiangfu0 / @walterddr please review. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @xiangfu0 to also take a look
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java
Outdated
Show resolved
Hide resolved
b744ebe
to
34d976c
Compare
Please review once more @Jackie-Jiang. Thanks |
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java
Outdated
Show resolved
Hide resolved
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a bugfix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed in commons-configuration2, as it does not support string escape/unescape like commons-configuration1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give an example of what the property will look like in v1 vs v2? Will v1 automatically escape/unescape the value?
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java
Outdated
Show resolved
Hide resolved
34d976c
to
e502b71
Compare
60988cd
to
3c9c15a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the great effort!
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/utils/SegmentMetadataUtils.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java
Outdated
Show resolved
Hide resolved
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give an example of what the property will look like in v1 vs v2? Will v1 automatically escape/unescape the value?
As per the issue and in continuation of the previous work #11792, #11868, #11916, This PR upgrade the
Metadata
tocommons-configuartion2
.