Migrate deprecated commons-configuration 1.x to commons-configuration 2.x (#4604)

* Migration to commons-configuration2 and commons-lang3

* Change test that breaks with commons configuration 2

* Add commons-beanutils dependency

* Revert "Change test that breaks with commons configuration 2"

This reverts commit 073c267.

* Fix ConfigKeyTest

* Configure list handling to match commons-configuration 1.x default behavior

* Fix the issue in using the API to configure defaults

* Configure list delimiter handler also for distributedlog's ConfigurationSubscription

* Address issue with system properties being null

* Use list delimiter handler in DistributedLogConfiguration too

* Fix spotbugs check

* Fix ConfigurationTest

- system properties are read when ServerConfiguration gets initialized

* Fix checkstyle

Author: lhotari

bookkeeper-benchmark/src/main/java/org/apache/bookkeeper/benchmark/BenchBookie.java

@@ -49,7 +49,7 @@
 import org.apache.commons.cli.Options;
 import org.apache.commons.cli.ParseException;
 import org.apache.commons.cli.PosixParser;
-import org.apache.commons.lang.SystemUtils;
+import org.apache.commons.lang3.SystemUtils;
 import org.apache.zookeeper.KeeperException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

bookkeeper-common/pom.xml

@@ -59,6 +59,10 @@
       <groupId>org.jctools</groupId>
       <artifactId>jctools-core</artifactId>
     </dependency>
+    <dependency>
+      <groupId>org.apache.commons</groupId>
+      <artifactId>commons-lang3</artifactId>
+    </dependency>
     <dependency>
       <groupId>io.netty.incubator</groupId>
       <artifactId>netty-incubator-transport-native-io_uring</artifactId>
@@ -90,11 +94,6 @@
       <version>${project.parent.version}</version>
       <scope>test</scope>
     </dependency>
-    <dependency>
-      <groupId>org.apache.commons</groupId>
-      <artifactId>commons-lang3</artifactId>
-      <scope>test</scope>
-    </dependency>
     <dependency>
       <groupId>org.awaitility</groupId>
       <artifactId>awaitility</artifactId>

bookkeeper-common/src/main/java/org/apache/bookkeeper/common/conf/ComponentConfiguration.java

@@ -21,17 +21,24 @@
 import java.math.BigDecimal;
 import java.math.BigInteger;
 import java.net.URL;
+import java.time.Duration;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
 import org.apache.bookkeeper.common.util.JsonUtil;
 import org.apache.bookkeeper.common.util.JsonUtil.ParseJsonException;
-import org.apache.commons.configuration.CompositeConfiguration;
-import org.apache.commons.configuration.Configuration;
-import org.apache.commons.configuration.ConfigurationException;
-import org.apache.commons.configuration.PropertiesConfiguration;
+import org.apache.commons.configuration2.CompositeConfiguration;
+import org.apache.commons.configuration2.Configuration;
+import org.apache.commons.configuration2.ConfigurationDecoder;
+import org.apache.commons.configuration2.ImmutableConfiguration;
+import org.apache.commons.configuration2.ex.ConfigurationException;
+import org.apache.commons.configuration2.interpol.ConfigurationInterpolator;
+import org.apache.commons.configuration2.interpol.Lookup;
+import org.apache.commons.configuration2.sync.LockMode;
+import org.apache.commons.configuration2.sync.Synchronizer;
 
 /**
  * Component Configuration.
@@ -70,7 +77,7 @@ public CompositeConfiguration getUnderlyingConf() {
      * @throws ConfigurationException when failed to load configuration.
      */
     public void loadConf(URL confURL) throws ConfigurationException {
-        Configuration loadedConf = new PropertiesConfiguration(confURL);
+        Configuration loadedConf = ConfigurationUtil.newConfiguration(conf -> conf.propertiesBuilder(confURL));
         loadConf(loadedConf);
     }
 
@@ -292,6 +299,128 @@ public List<Object> getList(String key, List<?> defaultValue) {
         return conf.getList(getKeyName(key), defaultValue);
     }
 
+    @Override
+    public ConfigurationInterpolator getInterpolator() {
+        return conf.getInterpolator();
+    }
+
+    @Override
+    public void installInterpolator(Map<String, ? extends Lookup> prefixLookups,
+                                    Collection<? extends Lookup> defLookups) {
+        conf.installInterpolator(prefixLookups, defLookups);
+    }
+
+    @Override
+    public void setInterpolator(ConfigurationInterpolator ci) {
+        conf.setInterpolator(ci);
+    }
+
+    @Override
+    public <T> T get(Class<T> cls, String key) {
+        return conf.get(cls, key);
+    }
+
+    @Override
+    public <T> T get(Class<T> cls, String key, T defaultValue) {
+        return conf.get(cls, key, defaultValue);
+    }
+
+    @Override
+    public Object getArray(Class<?> cls, String key) {
+        return conf.getArray(cls, key);
+    }
+
+    @Override
+    @Deprecated
+    public Object getArray(Class<?> cls, String key, Object defaultValue) {
+        return conf.getArray(cls, key, defaultValue);
+    }
+
+    @Override
+    public <T> Collection<T> getCollection(Class<T> cls, String key, Collection<T> target) {
+        return conf.getCollection(cls, key, target);
+    }
+
+    @Override
+    public <T> Collection<T> getCollection(Class<T> cls, String key, Collection<T> target, Collection<T> defaultValue) {
+        return conf.getCollection(cls, key, target, defaultValue);
+    }
+
+    @Override
+    public Duration getDuration(String key) {
+        return conf.getDuration(key);
+    }
+
+    @Override
+    public Duration getDuration(String key, Duration defaultValue) {
+        return conf.getDuration(key, defaultValue);
+    }
+
+    @Override
+    public String getEncodedString(String key) {
+        return conf.getEncodedString(key);
+    }
+
+    @Override
+    public String getEncodedString(String key, ConfigurationDecoder decoder) {
+        return conf.getEncodedString(key, decoder);
+    }
+
+    @Override
+    public <T extends Enum<T>> T getEnum(String key, Class<T> enumType) {
+        return conf.getEnum(key, enumType);
+    }
+
+    @Override
+    public <T extends Enum<T>> T getEnum(String key, Class<T> enumType, T defaultValue) {
+        return conf.getEnum(key, enumType, defaultValue);
+    }
+
+    @Override
+    public Iterator<String> getKeys(String prefix, String delimiter) {
+        return conf.getKeys(prefix, delimiter);
+    }
+
+    @Override
+    public <T> List<T> getList(Class<T> cls, String key) {
+        return conf.getList(cls, key);
+    }
+
+    @Override
+    public <T> List<T> getList(Class<T> cls, String key, List<T> defaultValue) {
+        return conf.getList(cls, key, defaultValue);
+    }
+
+    @Override
+    public ImmutableConfiguration immutableSubset(String prefix) {
+        return conf.immutableSubset(prefix);
+    }
+
+    @Override
+    public int size() {
+        return conf.size();
+    }
+
+    @Override
+    public Synchronizer getSynchronizer() {
+        return conf.getSynchronizer();
+    }
+
+    @Override
+    public void lock(LockMode mode) {
+        conf.lock(mode);
+    }
+
+    @Override
+    public void setSynchronizer(Synchronizer sync) {
+        conf.setSynchronizer(sync);
+    }
+
+    @Override
+    public void unlock(LockMode mode) {
+        conf.unlock(mode);
+    }
+
     /**
      * returns the string representation of json format of this config.
      *

bookkeeper-common/src/main/java/org/apache/bookkeeper/common/conf/ConcurrentConfiguration.java

@@ -23,7 +23,8 @@
 import java.util.Iterator;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
-import org.apache.commons.configuration.AbstractConfiguration;
+import org.apache.commons.configuration2.AbstractConfiguration;
+import org.apache.commons.configuration2.convert.DefaultListDelimiterHandler;
 
 /**
  * Configuration view built on concurrent hash map for fast thread-safe access.
@@ -38,6 +39,7 @@ public class ConcurrentConfiguration extends AbstractConfiguration {
     private final ConcurrentMap<String, Object> map;
 
     public ConcurrentConfiguration() {
+        setListDelimiterHandler(new DefaultListDelimiterHandler(','));
         this.map = new ConcurrentHashMap<>();
     }
 
@@ -48,22 +50,27 @@ protected void addPropertyDirect(String key, Object value) {
     }
 
     @Override
-    public Object getProperty(String key) {
+    public Object getPropertyInternal(String key) {
         return map.get(key);
     }
 
     @Override
-    public Iterator getKeys() {
+    public Iterator getKeysInternal() {
         return map.keySet().iterator();
     }
 
     @Override
-    public boolean containsKey(String key) {
+    public boolean containsKeyInternal(String key) {
         return map.containsKey(key);
     }
 
     @Override
-    public boolean isEmpty() {
+    protected boolean containsValueInternal(Object o) {
+        return map.containsValue(o);
+    }
+
+    @Override
+    public boolean isEmptyInternal() {
         return map.isEmpty();
     }
 

bookkeeper-common/src/main/java/org/apache/bookkeeper/common/conf/ConfigDef.java

@@ -40,8 +40,8 @@
 import java.util.stream.IntStream;
 import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;
-import org.apache.commons.configuration.Configuration;
-import org.apache.commons.lang.StringUtils;
+import org.apache.commons.configuration2.Configuration;
+import org.apache.commons.lang3.StringUtils;
 
 /**
  * A definition of a configuration instance.

bookkeeper-common/src/main/java/org/apache/bookkeeper/common/conf/ConfigKey.java

@@ -33,8 +33,8 @@
 import org.apache.bookkeeper.common.annotation.InterfaceAudience.Public;
 import org.apache.bookkeeper.common.conf.validators.NullValidator;
 import org.apache.bookkeeper.common.util.ReflectionUtils;
-import org.apache.commons.configuration.Configuration;
-import org.apache.commons.configuration.ConfigurationException;
+import org.apache.commons.configuration2.Configuration;
+import org.apache.commons.configuration2.ex.ConfigurationException;
 
 /**
  * A configuration key in a configuration.

bookkeeper-common/src/main/java/org/apache/bookkeeper/common/conf/ConfigurationUtil.java

@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.bookkeeper.common.conf;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.function.Function;
+import org.apache.commons.configuration2.Configuration;
+import org.apache.commons.configuration2.MapConfiguration;
+import org.apache.commons.configuration2.PropertiesConfiguration;
+import org.apache.commons.configuration2.builder.BasicBuilderParameters;
+import org.apache.commons.configuration2.builder.DefaultParametersManager;
+import org.apache.commons.configuration2.builder.FileBasedConfigurationBuilder;
+import org.apache.commons.configuration2.builder.fluent.Configurations;
+import org.apache.commons.configuration2.builder.fluent.Parameters;
+import org.apache.commons.configuration2.convert.DefaultListDelimiterHandler;
+import org.apache.commons.configuration2.ex.ConfigurationException;
+
+public class ConfigurationUtil {
+    /**
+     * Create a new PropertiesConfiguration using the given builder function.
+     * The purpose of this method is to configure the list handling to behave in the same way as in
+     * commons-configuration 1.x. without duplicating the code in multiple places.
+     *
+     * @param builderFunction a function that takes a Configurations object and returns a FileBasedConfigurationBuilder
+     * @return a new PropertiesConfiguration
+     * @throws ConfigurationException if there is an error creating the configuration
+     */
+    public static PropertiesConfiguration newConfiguration(
+            Function<Configurations, FileBasedConfigurationBuilder<PropertiesConfiguration>> builderFunction)
+            throws ConfigurationException {
+        // configure defaults
+        DefaultParametersManager parametersManager = new DefaultParametersManager();
+        parametersManager.registerDefaultsHandler(BasicBuilderParameters.class, basicBuilderParameters -> {
+            // configure list handling to behave in the same way as in commons-configuration 1.x
+            basicBuilderParameters.setListDelimiterHandler(new DefaultListDelimiterHandler(','));
+        });
+        Parameters params = new Parameters(parametersManager);
+        Configurations configurations = new Configurations(params);
+        // create a new builder using the provided function
+        FileBasedConfigurationBuilder<PropertiesConfiguration> builder = builderFunction.apply(configurations);
+        // build the configuration
+        return builder.getConfiguration();
+    }
+
+    /**
+     * Read system properties as a Configuration. Where a copy of the system properties is made
+     * so that mutations to the Configuration do not affect the system properties.
+     *
+     * @return a Configuration object containing the system properties
+     */
+    public static Configuration readSystemPropertiesAsConfiguration() {
+        Properties properties = System.getProperties();
+        Map<String, Object> propertiesMap = new HashMap<String, Object>();
+        for (Map.Entry<Object, Object> entry : properties.entrySet()) {
+            propertiesMap.put(entry.getKey().toString(), entry.getValue());
+        }
+        return new MapConfiguration(propertiesMap);
+    }
+}

bookkeeper-common/src/main/java/org/apache/bookkeeper/common/net/ServiceURI.java

@@ -34,7 +34,7 @@
 import lombok.RequiredArgsConstructor;
 import org.apache.bookkeeper.common.annotation.InterfaceAudience.Public;
 import org.apache.bookkeeper.common.annotation.InterfaceStability.Evolving;
-import org.apache.commons.lang.StringUtils;
+import org.apache.commons.lang3.StringUtils;
 
 /**
  * ServiceURI represents service uri within bookkeeper cluster.

bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedExecutor.java

@@ -45,7 +45,7 @@
 import org.apache.bookkeeper.stats.OpStatsLogger;
 import org.apache.bookkeeper.stats.StatsLogger;
 import org.apache.bookkeeper.stats.ThreadRegistry;
-import org.apache.commons.lang.StringUtils;
+import org.apache.commons.lang3.StringUtils;
 import org.slf4j.MDC;
 
 /**

bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/ReflectionUtils.java

@@ -21,8 +21,8 @@
 import java.lang.reflect.Constructor;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
-import org.apache.commons.configuration.Configuration;
-import org.apache.commons.configuration.ConfigurationException;
+import org.apache.commons.configuration2.Configuration;
+import org.apache.commons.configuration2.ex.ConfigurationException;
 
 /**
  * General Class Reflection Utils.