This is an automated email from the ASF dual-hosted git repository. dlmarion pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/main by this push: new 07a08424d4 Revert recent configuration commits, implement differently (#2864) 07a08424d4 is described below commit 07a08424d4ce3f3bb2608f4a83f5eb07e3418f9a Author: Dave Marion <dlmar...@apache.org> AuthorDate: Thu Aug 11 07:03:42 2022 -0400 Revert recent configuration commits, implement differently (#2864) * Revert "Add missing logic to Configuration.getProperties (#2859)" This reverts commit 22ae64299580678f75888d9c68ec01887e34796c. * Revert "Provide getProperties method on Configuration that does not filter (#2834)" This reverts commit fb3aeb961393023d6a652d3f51be2e42ec2d1134. * Optimized AccumuloConfiguration.get(String) and reduced calls to it Optimized get(String) by first attempting to get a Property from the String parameter, then perform an efficient lookup using the Property. If that fails (ClientProperty, custom property, etc), then fall back to filtering. --- .../accumulo/core/classloader/ClassLoaderUtil.java | 2 +- .../core/clientImpl/ClientConfConverter.java | 15 ---------- .../accumulo/core/conf/AccumuloConfiguration.java | 34 +++++++++------------- .../accumulo/core/conf/ConfigurationCopy.java | 14 --------- .../accumulo/core/conf/DefaultConfiguration.java | 14 --------- .../accumulo/core/conf/SiteConfiguration.java | 15 ---------- .../core/conf/AccumuloConfigurationTest.java | 13 --------- .../server/conf/NamespaceConfiguration.java | 2 +- .../server/conf/RuntimeFixedProperties.java | 2 +- .../server/conf/ZooBasedConfiguration.java | 16 ---------- .../conf/ServerConfigurationFactoryTest.java | 5 ++-- .../server/conf/TableConfigurationTest.java | 2 +- .../server/conf/ZooBasedConfigurationTest.java | 2 +- .../tserver/replication/AccumuloReplicaSystem.java | 3 +- .../accumulo/test/functional/ReadWriteIT.java | 2 +- 15 files changed, 23 insertions(+), 118 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java b/core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java index f58b1a1bfb..47e7ef584a 100644 --- a/core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java +++ b/core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java @@ -39,7 +39,7 @@ public class ClassLoaderUtil { public static synchronized void initContextFactory(AccumuloConfiguration conf) { if (FACTORY == null) { LOG.debug("Creating {}", ContextClassLoaderFactory.class.getName()); - String factoryName = conf.get(Property.GENERAL_CONTEXT_CLASSLOADER_FACTORY.getKey()); + String factoryName = conf.get(Property.GENERAL_CONTEXT_CLASSLOADER_FACTORY); if (factoryName == null || factoryName.isEmpty()) { // load the default implementation LOG.info("Using default {}, which is subject to change in a future release", diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/ClientConfConverter.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/ClientConfConverter.java index 865f9ad9f0..f7792468bb 100644 --- a/core/src/main/java/org/apache/accumulo/core/clientImpl/ClientConfConverter.java +++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/ClientConfConverter.java @@ -202,21 +202,6 @@ public class ClientConfConverter { } } - @Override - public void getProperties(Map<String,String> props, String... properties) { - defaults.getProperties(props, properties); - if (properties == null || properties.length == 0) { - config.getKeys().forEachRemaining(key -> props.put(key, config.getString(key))); - } else { - for (String p : properties) { - String value = config.getString(p); - if (value != null) { - props.put(p, value); - } - } - } - } - @Override public void getProperties(Map<String,String> props, Predicate<String> filter) { defaults.getProperties(props, filter); diff --git a/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java b/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java index 2600e5a297..6fdfe69159 100644 --- a/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java +++ b/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java @@ -26,6 +26,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.Objects; import java.util.Optional; import java.util.OptionalInt; import java.util.TreeMap; @@ -71,18 +72,22 @@ public abstract class AccumuloConfiguration implements Iterable<Entry<String,Str * Gets a property value from this configuration. * * <p> - * Note: this is inefficient, but convenient on occasion. For retrieving multiple properties, use - * {@link #getProperties(Map, String...)} if the property names are known or - * {@link #getProperties(Map, Predicate)} with a custom filter. + * Note: this is inefficient for values that are not a {@link Property}. For retrieving multiple + * properties, use {@link #getProperties(Map, Predicate)} with a custom filter. * * @param property * property to get * @return property value */ public String get(String property) { - Map<String,String> propMap = new HashMap<>(1); - getProperties(propMap, property); - return propMap.get(property); + try { + return get(Property.valueOf(property)); + } catch (IllegalArgumentException e) { + // Could be a client or custom property, fall back to filtering + Map<String,String> propMap = new HashMap<>(1); + getProperties(propMap, key -> Objects.equals(property, key)); + return propMap.get(property); + } } /** @@ -119,25 +124,11 @@ public abstract class AccumuloConfiguration implements Iterable<Entry<String,Str : Stream.of(deprecated).filter(this::isPropertySet).findFirst().orElse(property); } - /** - * Returns property key/value pairs in this configuration and parent configuration. - * - * @param props - * properties object to populate - * @param properties - * property key/values to copy to the props map. Copies all properties if null. - */ - public abstract void getProperties(Map<String,String> props, String... properties); - /** * Returns property key/value pairs in this configuration. The pairs include those defined in this * configuration which pass the given filter, and those supplied from the parent configuration * which are not included from here. * - * <p> - * Note: this is inefficient for retrieving fully-qualified properties, use - * {@link #getProperties(Map, String...)} instead. - * * @param props * properties object to populate * @param filter @@ -153,8 +144,9 @@ public abstract class AccumuloConfiguration implements Iterable<Entry<String,Str */ @Override public Iterator<Entry<String,String>> iterator() { + Predicate<String> all = x -> true; TreeMap<String,String> entries = new TreeMap<>(); - getProperties(entries); + getProperties(entries, all); return entries.entrySet().iterator(); } diff --git a/core/src/main/java/org/apache/accumulo/core/conf/ConfigurationCopy.java b/core/src/main/java/org/apache/accumulo/core/conf/ConfigurationCopy.java index 9f176e9bdf..221934c7c2 100644 --- a/core/src/main/java/org/apache/accumulo/core/conf/ConfigurationCopy.java +++ b/core/src/main/java/org/apache/accumulo/core/conf/ConfigurationCopy.java @@ -75,20 +75,6 @@ public class ConfigurationCopy extends AccumuloConfiguration { return copy.get(property.getKey()); } - @Override - public void getProperties(Map<String,String> props, String... properties) { - if (properties == null || properties.length == 0) { - props.putAll(copy); - } else { - for (String p : properties) { - String value = copy.get(p); - if (value != null) { - props.put(p, value); - } - } - } - } - @Override public void getProperties(Map<String,String> props, Predicate<String> filter) { for (Entry<String,String> entry : copy.entrySet()) { diff --git a/core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java b/core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java index 9bdc617085..2c3abca0f1 100644 --- a/core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java +++ b/core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java @@ -54,20 +54,6 @@ public class DefaultConfiguration extends AccumuloConfiguration { return resolvedProps.get(property.getKey()); } - @Override - public void getProperties(Map<String,String> props, String... properties) { - if (properties == null || properties.length == 0) { - props.putAll(resolvedProps); - } else { - for (String p : properties) { - String value = resolvedProps.get(p); - if (value != null) { - props.put(p, value); - } - } - } - } - @Override public void getProperties(Map<String,String> props, Predicate<String> filter) { resolvedProps.entrySet().stream().filter(p -> filter.test(p.getKey())) diff --git a/core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java b/core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java index bb936ce329..c20aab006b 100644 --- a/core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java +++ b/core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java @@ -261,21 +261,6 @@ public class SiteConfiguration extends AccumuloConfiguration { return config.containsKey(prop.getKey()) || parent.isPropertySet(prop); } - @Override - public void getProperties(Map<String,String> props, String... properties) { - parent.getProperties(props, properties); - if (properties == null || properties.length == 0) { - props.putAll(config); - } else { - for (String p : properties) { - String value = config.get(p); - if (value != null) { - props.put(p, value); - } - } - } - } - @Override public void getProperties(Map<String,String> props, Predicate<String> filter) { getProperties(props, filter, true); diff --git a/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java b/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java index b8ca9b2c7e..aa3980eb32 100644 --- a/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java +++ b/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java @@ -173,19 +173,6 @@ public class AccumuloConfigurationTest { return v; } - @Override - public void getProperties(Map<String,String> props, String... properties) { - if (parent != null) { - parent.getProperties(props, properties); - } - for (String p : properties) { - String value = props.get(p); - if (value != null) { - props.put(p, value); - } - } - } - @Override public void getProperties(Map<String,String> output, Predicate<String> filter) { if (parent != null) { diff --git a/server/base/src/main/java/org/apache/accumulo/server/conf/NamespaceConfiguration.java b/server/base/src/main/java/org/apache/accumulo/server/conf/NamespaceConfiguration.java index 2761fb3211..99193a12b3 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/conf/NamespaceConfiguration.java +++ b/server/base/src/main/java/org/apache/accumulo/server/conf/NamespaceConfiguration.java @@ -59,7 +59,7 @@ public class NamespaceConfiguration extends ZooBasedConfiguration { return value; } - return getParent().get(key); + return getParent().get(property); } /** diff --git a/server/base/src/main/java/org/apache/accumulo/server/conf/RuntimeFixedProperties.java b/server/base/src/main/java/org/apache/accumulo/server/conf/RuntimeFixedProperties.java index 2497fecd76..53feff629d 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/conf/RuntimeFixedProperties.java +++ b/server/base/src/main/java/org/apache/accumulo/server/conf/RuntimeFixedProperties.java @@ -64,7 +64,7 @@ public class RuntimeFixedProperties { origStored.put(key, value); } else { // Not in ZK, use config or default. - value = siteConfig.get(key); + value = siteConfig.get(prop); } fixed.put(key, value); log.trace("fixed property name: {} = {}", key, value); diff --git a/server/base/src/main/java/org/apache/accumulo/server/conf/ZooBasedConfiguration.java b/server/base/src/main/java/org/apache/accumulo/server/conf/ZooBasedConfiguration.java index beb3044088..15126c806a 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/conf/ZooBasedConfiguration.java +++ b/server/base/src/main/java/org/apache/accumulo/server/conf/ZooBasedConfiguration.java @@ -121,22 +121,6 @@ public class ZooBasedConfiguration extends AccumuloConfiguration { return null; } - @Override - public void getProperties(Map<String,String> props, String... properties) { - parent.getProperties(props, properties); - Map<String,String> theseProps = getSnapshot(); - if (properties == null || properties.length == 0) { - props.putAll(theseProps); - } else { - for (String p : properties) { - String value = theseProps.get(p); - if (value != null) { - props.put(p, value); - } - } - } - } - @Override public void getProperties(final Map<String,String> props, final Predicate<String> filter) { diff --git a/server/base/src/test/java/org/apache/accumulo/server/conf/ServerConfigurationFactoryTest.java b/server/base/src/test/java/org/apache/accumulo/server/conf/ServerConfigurationFactoryTest.java index d1ed1671d0..71e13cda8a 100644 --- a/server/base/src/test/java/org/apache/accumulo/server/conf/ServerConfigurationFactoryTest.java +++ b/server/base/src/test/java/org/apache/accumulo/server/conf/ServerConfigurationFactoryTest.java @@ -20,7 +20,6 @@ package org.apache.accumulo.server.conf; import static org.easymock.EasyMock.anyObject; import static org.easymock.EasyMock.createMock; -import static org.easymock.EasyMock.createNiceMock; import static org.easymock.EasyMock.eq; import static org.easymock.EasyMock.expect; import static org.easymock.EasyMock.expectLastCall; @@ -75,7 +74,9 @@ public class ServerConfigurationFactoryTest { propStore.registerAsListener(anyObject(), anyObject()); expectLastCall().anyTimes(); - sysConfig = createNiceMock(SystemConfiguration.class); + sysConfig = createMock(SystemConfiguration.class); + sysConfig.getProperties(anyObject(), anyObject()); + expectLastCall().anyTimes(); context = createMock(ServerContext.class); expect(context.getZooKeeperRoot()).andReturn("/accumulo/" + IID).anyTimes(); diff --git a/server/base/src/test/java/org/apache/accumulo/server/conf/TableConfigurationTest.java b/server/base/src/test/java/org/apache/accumulo/server/conf/TableConfigurationTest.java index 5e4358b100..9c60673773 100644 --- a/server/base/src/test/java/org/apache/accumulo/server/conf/TableConfigurationTest.java +++ b/server/base/src/test/java/org/apache/accumulo/server/conf/TableConfigurationTest.java @@ -164,7 +164,7 @@ public class TableConfigurationTest { nsConfig.zkChangeEvent(NamespacePropKey.of(instanceId, NID)); assertEquals("123", tableConfig.get(TABLE_FILE_MAX)); // from ns - assertEquals("aPassword1", tableConfig.get(INSTANCE_SECRET.getKey())); // from sys + assertEquals("aPassword1", tableConfig.get(INSTANCE_SECRET)); // from sys verify(propStore); } diff --git a/server/base/src/test/java/org/apache/accumulo/server/conf/ZooBasedConfigurationTest.java b/server/base/src/test/java/org/apache/accumulo/server/conf/ZooBasedConfigurationTest.java index 0881c41c44..8859dd70a0 100644 --- a/server/base/src/test/java/org/apache/accumulo/server/conf/ZooBasedConfigurationTest.java +++ b/server/base/src/test/java/org/apache/accumulo/server/conf/ZooBasedConfigurationTest.java @@ -240,7 +240,7 @@ public class ZooBasedConfigurationTest { // get from "default" - root parent config assertEquals(TABLE_BLOOM_SIZE.getDefaultValue(), tableConfig.get(TABLE_BLOOM_SIZE)); - assertEquals(TABLE_DURABILITY.getDefaultValue(), tableConfig.get(TABLE_DURABILITY.getKey())); + assertEquals(TABLE_DURABILITY.getDefaultValue(), tableConfig.get(TABLE_DURABILITY)); // test getProperties Map<String,String> props = new HashMap<>(); diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java index de168bd2a8..fce65ac785 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java @@ -134,8 +134,7 @@ public class AccumuloReplicaSystem implements ReplicaSystem { final ReplicaSystemHelper helper) { final AccumuloConfiguration localConf = conf; - log.debug("Replication RPC timeout is {}", - localConf.get(Property.REPLICATION_RPC_TIMEOUT.getKey())); + log.debug("Replication RPC timeout is {}", localConf.get(Property.REPLICATION_RPC_TIMEOUT)); final String principal = getPrincipal(localConf, target); final File keytab; diff --git a/test/src/main/java/org/apache/accumulo/test/functional/ReadWriteIT.java b/test/src/main/java/org/apache/accumulo/test/functional/ReadWriteIT.java index bd01153b38..e4f43d346b 100644 --- a/test/src/main/java/org/apache/accumulo/test/functional/ReadWriteIT.java +++ b/test/src/main/java/org/apache/accumulo/test/functional/ReadWriteIT.java @@ -146,7 +146,7 @@ public class ReadWriteIT extends AccumuloClusterHarness { } if (getCluster() instanceof StandaloneAccumuloCluster) { String monitorSslKeystore = - getCluster().getSiteConfiguration().get(Property.MONITOR_SSL_KEYSTORE.getKey()); + getCluster().getSiteConfiguration().get(Property.MONITOR_SSL_KEYSTORE); if (monitorSslKeystore != null && !monitorSslKeystore.isEmpty()) { log.info( "Using HTTPS since monitor ssl keystore configuration was observed in accumulo configuration");