You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@accumulo.apache.org by dl...@apache.org on 2022/08/11 11:03:47 UTC
[accumulo] branch main updated: Revert recent configuration commits, implement differently (#2864)
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 <dl...@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");