From c477ebd4d1b49cb19e14f26287c06a0c531c00f2 Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Tue, 24 Oct 2023 22:36:59 -0400 Subject: [PATCH 1/6] 11085: PinotConfig commons-configuartions2 upgrade --- pinot-spi/pom.xml | 14 +++++++++++ .../spi/env/CommonsConfigurationUtils.java | 23 +++++++++++++++++++ .../apache/pinot/tools/QuickStartBase.java | 2 +- .../command/AbstractBaseAdminCommand.java | 2 +- .../admin/command/StartBrokerCommand.java | 2 +- .../admin/command/StartControllerCommand.java | 2 +- .../admin/command/StartMinionCommand.java | 2 +- .../admin/command/StartServerCommand.java | 2 +- .../PinotServiceManagerInstanceResource.java | 2 +- .../pinot/tools/utils/PinotConfigUtils.java | 10 ++++---- 10 files changed, 50 insertions(+), 11 deletions(-) diff --git a/pinot-spi/pom.xml b/pinot-spi/pom.xml index 08e5f6709df..40970869327 100644 --- a/pinot-spi/pom.xml +++ b/pinot-spi/pom.xml @@ -62,6 +62,20 @@ + + org.apache.commons + commons-configuration2 + + + commons-logging + commons-logging + + + commons-lang + commons-lang + + + commons-codec commons-codec diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java b/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java index 48521570808..8481e04c96d 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java @@ -105,6 +105,11 @@ public static Stream getKeysStream(Configuration configuration) { return StreamSupport.stream(getIterable(configuration.getKeys()).spliterator(), false); } + public static Stream getKeysStream(org.apache.commons.configuration2.Configuration configuration) { + return StreamSupport.stream(getIterable(configuration.getKeys()).spliterator(), false); + } + + /** * Provides a list of all the keys found in a {@link Configuration}. * @param configuration to iterate on keys @@ -121,6 +126,10 @@ public static Map toMap(Configuration configuration) { return getKeysStream(configuration).collect(Collectors.toMap(key -> key, key -> mapValue(key, configuration))); } + public static Map toMap(org.apache.commons.configuration2.Configuration configuration) { + return getKeysStream(configuration).collect(Collectors.toMap(key -> key, key -> mapValue(key, configuration))); + } + private static Object mapValue(String key, Configuration configuration) { // For multi-value config, convert it to a single comma connected string value. // For single-value config, return its raw property, unless it needs interpolation. @@ -135,6 +144,20 @@ private static Object mapValue(String key, Configuration configuration) { }); } + private static Object mapValue(String key, org.apache.commons.configuration2.Configuration configuration) { + // For multi-value config, convert it to a single comma connected string value. + // For single-value config, return its raw property, unless it needs interpolation. + return Optional.of(configuration.getStringArray(key)).filter(values -> values.length > 1) + .map(values -> Arrays.stream(values).collect(Collectors.joining(","))).orElseGet(() -> { + Object rawProperty = configuration.getProperty(key); + if (!needInterpolate(rawProperty)) { + return rawProperty; + } + // The string value is converted to the requested type when accessing it via PinotConfiguration. + return configuration.getString(key); + }); + } + public static boolean needInterpolate(Object rawProperty) { if (rawProperty instanceof String) { String strProperty = (String) rawProperty; diff --git a/pinot-tools/src/main/java/org/apache/pinot/tools/QuickStartBase.java b/pinot-tools/src/main/java/org/apache/pinot/tools/QuickStartBase.java index 1485902024e..11aadf4c994 100644 --- a/pinot-tools/src/main/java/org/apache/pinot/tools/QuickStartBase.java +++ b/pinot-tools/src/main/java/org/apache/pinot/tools/QuickStartBase.java @@ -31,7 +31,7 @@ import java.util.List; import java.util.Map; import java.util.Properties; -import org.apache.commons.configuration.ConfigurationException; +import org.apache.commons.configuration2.ex.ConfigurationException; import org.apache.commons.io.FileUtils; import org.apache.commons.io.LineIterator; import org.apache.commons.lang3.StringUtils; diff --git a/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/AbstractBaseAdminCommand.java b/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/AbstractBaseAdminCommand.java index 7ac6e85d7bd..a22b97c4dea 100644 --- a/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/AbstractBaseAdminCommand.java +++ b/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/AbstractBaseAdminCommand.java @@ -32,7 +32,7 @@ import java.util.List; import java.util.Map; import javax.annotation.Nullable; -import org.apache.commons.configuration.ConfigurationException; +import org.apache.commons.configuration2.ex.ConfigurationException; import org.apache.commons.lang3.StringUtils; import org.apache.http.Header; import org.apache.pinot.common.auth.AuthProviderUtils; diff --git a/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartBrokerCommand.java b/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartBrokerCommand.java index bc71bb113f8..2ca6488c3b7 100644 --- a/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartBrokerCommand.java +++ b/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartBrokerCommand.java @@ -24,7 +24,7 @@ import java.util.HashMap; import java.util.Map; import org.apache.commons.collections.MapUtils; -import org.apache.commons.configuration.ConfigurationException; +import org.apache.commons.configuration2.ex.ConfigurationException; import org.apache.pinot.spi.services.ServiceRole; import org.apache.pinot.spi.utils.CommonConstants; import org.apache.pinot.tools.Command; diff --git a/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartControllerCommand.java b/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartControllerCommand.java index 1de1ca80f13..68045cc1421 100644 --- a/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartControllerCommand.java +++ b/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartControllerCommand.java @@ -23,7 +23,7 @@ import java.net.UnknownHostException; import java.util.HashMap; import java.util.Map; -import org.apache.commons.configuration.ConfigurationException; +import org.apache.commons.configuration2.ex.ConfigurationException; import org.apache.pinot.controller.ControllerConf; import org.apache.pinot.spi.services.ServiceRole; import org.apache.pinot.spi.utils.NetUtils; diff --git a/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartMinionCommand.java b/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartMinionCommand.java index 9a8e926b91b..6daca4c0e0d 100644 --- a/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartMinionCommand.java +++ b/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartMinionCommand.java @@ -24,7 +24,7 @@ import java.util.HashMap; import java.util.Map; import org.apache.commons.collections.MapUtils; -import org.apache.commons.configuration.ConfigurationException; +import org.apache.commons.configuration2.ex.ConfigurationException; import org.apache.pinot.spi.services.ServiceRole; import org.apache.pinot.spi.utils.CommonConstants; import org.apache.pinot.tools.Command; diff --git a/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServerCommand.java b/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServerCommand.java index 2fa5d0cb45b..64be41489f8 100644 --- a/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServerCommand.java +++ b/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServerCommand.java @@ -24,7 +24,7 @@ import java.util.HashMap; import java.util.Map; import org.apache.commons.collections.MapUtils; -import org.apache.commons.configuration.ConfigurationException; +import org.apache.commons.configuration2.ex.ConfigurationException; import org.apache.pinot.spi.services.ServiceRole; import org.apache.pinot.spi.utils.CommonConstants; import org.apache.pinot.tools.Command; diff --git a/pinot-tools/src/main/java/org/apache/pinot/tools/service/api/resources/PinotServiceManagerInstanceResource.java b/pinot-tools/src/main/java/org/apache/pinot/tools/service/api/resources/PinotServiceManagerInstanceResource.java index 4e4451bf3a6..6f7f6668ce1 100644 --- a/pinot-tools/src/main/java/org/apache/pinot/tools/service/api/resources/PinotServiceManagerInstanceResource.java +++ b/pinot-tools/src/main/java/org/apache/pinot/tools/service/api/resources/PinotServiceManagerInstanceResource.java @@ -45,7 +45,7 @@ import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; -import org.apache.commons.configuration.Configuration; +import org.apache.commons.configuration2.Configuration; import org.apache.pinot.controller.ControllerConf; import org.apache.pinot.spi.env.CommonsConfigurationUtils; import org.apache.pinot.spi.services.ServiceRole; diff --git a/pinot-tools/src/main/java/org/apache/pinot/tools/utils/PinotConfigUtils.java b/pinot-tools/src/main/java/org/apache/pinot/tools/utils/PinotConfigUtils.java index 403b0f07528..13df42161cf 100644 --- a/pinot-tools/src/main/java/org/apache/pinot/tools/utils/PinotConfigUtils.java +++ b/pinot-tools/src/main/java/org/apache/pinot/tools/utils/PinotConfigUtils.java @@ -27,8 +27,8 @@ import java.util.List; import java.util.Map; import java.util.Optional; -import org.apache.commons.configuration.ConfigurationException; -import org.apache.commons.configuration.PropertiesConfiguration; +import org.apache.commons.configuration2.builder.fluent.Configurations; +import org.apache.commons.configuration2.ex.ConfigurationException; import org.apache.commons.lang.StringUtils; import org.apache.pinot.controller.ControllerConf; import org.apache.pinot.controller.ControllerConf.ControllerPeriodicTasksConf; @@ -98,7 +98,8 @@ public static Map readControllerConfigFromFile(String configFile return null; } - Map properties = CommonsConfigurationUtils.toMap(new PropertiesConfiguration(configFile)); + Configurations configs = new Configurations(); + Map properties = CommonsConfigurationUtils.toMap(configs.properties(configFile)); ControllerConf conf = new ControllerConf(properties); conf.setPinotFSFactoryClasses(null); @@ -142,7 +143,8 @@ public static Map readConfigFromFile(String configFileName) } File configFile = new File(configFileName); if (configFile.exists()) { - return CommonsConfigurationUtils.toMap(new PropertiesConfiguration(configFile)); + Configurations configs = new Configurations(); + return CommonsConfigurationUtils.toMap(configs.properties(configFile)); } return null; From 8588f1e4cc48aebcafecf9fd90830dc8aeff55fb Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Wed, 25 Oct 2023 21:37:40 -0400 Subject: [PATCH 2/6] Updated for failing integration test. --- .../apache/pinot/integration/tests/TlsIntegrationTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java index 177b442a9c1..3a70cc591d9 100644 --- a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java +++ b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java @@ -33,7 +33,7 @@ import java.util.Objects; import java.util.Properties; import java.util.stream.Collectors; -import org.apache.commons.configuration.ConfigurationException; +import org.apache.commons.configuration2.ex.ConfigurationException; import org.apache.commons.io.FileUtils; import org.apache.helix.model.InstanceConfig; import org.apache.http.Header; @@ -211,7 +211,7 @@ protected PinotConfiguration getDefaultServerConfiguration() { CertBasedTlsChannelAccessControlFactory.class.getName()); prop.put("pinot.server.adminapi.access.protocols", "internal"); prop.put("pinot.server.adminapi.access.protocols.internal.protocol", "https"); - prop.put("pinot.server.adminapi.access.protocols.internal.port", "7443"); + prop.put("pinot.server.adminapi.access.protocols.internal.port", "9443"); prop.put("pinot.server.netty.enabled", "false"); prop.put("pinot.server.nettytls.enabled", "true"); prop.put("pinot.server.nettytls.port", "8089"); From 9a925bc7a8681a76182f3e0aba806c3378fa229e Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Fri, 27 Oct 2023 05:02:03 -0400 Subject: [PATCH 3/6] Revert the unwanted changes and Derpecated annotation to old functions. --- .../org/apache/pinot/integration/tests/TlsIntegrationTest.java | 2 +- .../org/apache/pinot/spi/env/CommonsConfigurationUtils.java | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java index 3a70cc591d9..374cd37953b 100644 --- a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java +++ b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java @@ -211,7 +211,7 @@ protected PinotConfiguration getDefaultServerConfiguration() { CertBasedTlsChannelAccessControlFactory.class.getName()); prop.put("pinot.server.adminapi.access.protocols", "internal"); prop.put("pinot.server.adminapi.access.protocols.internal.protocol", "https"); - prop.put("pinot.server.adminapi.access.protocols.internal.port", "9443"); + prop.put("pinot.server.adminapi.access.protocols.internal.port", "7443"); prop.put("pinot.server.netty.enabled", "false"); prop.put("pinot.server.nettytls.enabled", "true"); prop.put("pinot.server.nettytls.port", "8089"); diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java b/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java index 8481e04c96d..1614deb3a7e 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java @@ -101,6 +101,7 @@ private static Iterable getIterable(Iterator keys) { * @param configuration to iterate on keys * @return a stream of keys */ + @Deprecated public static Stream getKeysStream(Configuration configuration) { return StreamSupport.stream(getIterable(configuration.getKeys()).spliterator(), false); } @@ -122,6 +123,7 @@ public static List getKeys(Configuration configuration) { /** * @return a key-value {@link Map} found in the provided {@link Configuration} */ + @Deprecated public static Map toMap(Configuration configuration) { return getKeysStream(configuration).collect(Collectors.toMap(key -> key, key -> mapValue(key, configuration))); } @@ -130,6 +132,7 @@ public static Map toMap(org.apache.commons.configuration2.Config return getKeysStream(configuration).collect(Collectors.toMap(key -> key, key -> mapValue(key, configuration))); } + @Deprecated private static Object mapValue(String key, Configuration configuration) { // For multi-value config, convert it to a single comma connected string value. // For single-value config, return its raw property, unless it needs interpolation. From d4d9d6eb5dbe628582c4ab57c4f1c638384e8c5d Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Fri, 27 Oct 2023 21:01:13 -0400 Subject: [PATCH 4/6] Changes as per the PR comment. --- .../pinot/spi/env/CommonsConfigurationUtils.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java b/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java index 1614deb3a7e..26729dfa070 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java @@ -32,7 +32,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; import java.util.stream.StreamSupport; -import org.apache.commons.configuration.Configuration; +import org.apache.commons.configuration2.Configuration; import org.apache.commons.configuration.ConfigurationException; import org.apache.commons.configuration.PropertiesConfiguration; import org.apache.commons.lang3.StringUtils; @@ -102,11 +102,11 @@ private static Iterable getIterable(Iterator keys) { * @return a stream of keys */ @Deprecated - public static Stream getKeysStream(Configuration configuration) { + public static Stream getKeysStream(org.apache.commons.configuration.Configuration configuration) { return StreamSupport.stream(getIterable(configuration.getKeys()).spliterator(), false); } - public static Stream getKeysStream(org.apache.commons.configuration2.Configuration configuration) { + public static Stream getKeysStream(Configuration configuration) { return StreamSupport.stream(getIterable(configuration.getKeys()).spliterator(), false); } @@ -124,16 +124,16 @@ public static List getKeys(Configuration configuration) { * @return a key-value {@link Map} found in the provided {@link Configuration} */ @Deprecated - public static Map toMap(Configuration configuration) { + public static Map toMap(org.apache.commons.configuration.Configuration configuration) { return getKeysStream(configuration).collect(Collectors.toMap(key -> key, key -> mapValue(key, configuration))); } - public static Map toMap(org.apache.commons.configuration2.Configuration configuration) { + public static Map toMap(Configuration configuration) { return getKeysStream(configuration).collect(Collectors.toMap(key -> key, key -> mapValue(key, configuration))); } @Deprecated - private static Object mapValue(String key, Configuration configuration) { + private static Object mapValue(String key, org.apache.commons.configuration.Configuration configuration) { // For multi-value config, convert it to a single comma connected string value. // For single-value config, return its raw property, unless it needs interpolation. return Optional.of(configuration.getStringArray(key)).filter(values -> values.length > 1) @@ -147,7 +147,7 @@ private static Object mapValue(String key, Configuration configuration) { }); } - private static Object mapValue(String key, org.apache.commons.configuration2.Configuration configuration) { + private static Object mapValue(String key, Configuration configuration) { // For multi-value config, convert it to a single comma connected string value. // For single-value config, return its raw property, unless it needs interpolation. return Optional.of(configuration.getStringArray(key)).filter(values -> values.length > 1) From 981a956a0e85d932c9788906300ad52298eb9ea7 Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Fri, 27 Oct 2023 21:35:50 -0400 Subject: [PATCH 5/6] fix linting issue. --- .../org/apache/pinot/spi/env/CommonsConfigurationUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java b/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java index 26729dfa070..1871ab60511 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java @@ -32,9 +32,9 @@ import java.util.stream.Collectors; import java.util.stream.Stream; import java.util.stream.StreamSupport; -import org.apache.commons.configuration2.Configuration; import org.apache.commons.configuration.ConfigurationException; import org.apache.commons.configuration.PropertiesConfiguration; +import org.apache.commons.configuration2.Configuration; import org.apache.commons.lang3.StringUtils; From 9bc5984021eafbbc9c89d73455813f9c4c820cf2 Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Fri, 27 Oct 2023 22:30:47 -0400 Subject: [PATCH 6/6] Fixing the compilation issue. --- .../org/apache/pinot/spi/env/CommonsConfigurationUtils.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java b/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java index 1871ab60511..4feb4d33c25 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java @@ -116,7 +116,7 @@ public static Stream getKeysStream(Configuration configuration) { * @param configuration to iterate on keys * @return a list of keys */ - public static List getKeys(Configuration configuration) { + public static List getKeys(org.apache.commons.configuration.Configuration configuration) { return getKeysStream(configuration).collect(Collectors.toList()); } @@ -172,7 +172,9 @@ public static boolean needInterpolate(Object rawProperty) { } @SuppressWarnings("unchecked") - public static T interpolate(Configuration configuration, String key, T defaultValue, Class returnType) { + @Deprecated + public static T interpolate(org.apache.commons.configuration.Configuration configuration, + String key, T defaultValue, Class returnType) { // Different from the generic getProperty() method, those type specific getters do config interpolation. if (Integer.class.equals(returnType)) { return (T) configuration.getInteger(key, (Integer) defaultValue);