Jackie-Jiang commented on code in PR #12393:
URL: https://github.com/apache/pinot/pull/12393#discussion_r1484929160
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -241,7 +241,11 @@ public static String
replaceSpecialCharacterInPropertyValue(String value) {
* {@link #replaceSpecialCharacterInPropertyValue(String)}.
*/
public static String recoverSpecialCharacterInPropertyValue(String value) {
- value = StringEscapeUtils.unescapeJava(value);
+ try {
+ value = StringEscapeUtils.unescapeJava(value);
+ } catch (Exception e) {
+ // If the value is not a valid escaped string, ignore the exception and
continue
Review Comment:
I don't think we want to log error here. If we want to find all old segments
that contains unescaped special character, a metric might be helpful.
IMO we can just leave it as is since the overhead is minimal.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -241,7 +241,11 @@ public static String
replaceSpecialCharacterInPropertyValue(String value) {
* {@link #replaceSpecialCharacterInPropertyValue(String)}.
*/
public static String recoverSpecialCharacterInPropertyValue(String value) {
- value = StringEscapeUtils.unescapeJava(value);
+ try {
Review Comment:
Please add some comments stating this is for backward compatibility, where
the old commons library escapes character in a different way
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
