This is an automated email from the ASF dual-hosted git repository.
ctubbsii pushed a commit to branch 2.1
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/2.1 by this push:
new 9bd6a8d403 Make isPropertySet work for fixed properties (#4752)
9bd6a8d403 is described below
commit 9bd6a8d40314626c9d1da0bec2f807640aff253d
Author: Christopher Tubbs <[email protected]>
AuthorDate: Wed Jul 24 09:05:52 2024 -0400
Make isPropertySet work for fixed properties (#4752)
* Add more comments (including javadocs) to explain fixed properties
* Add the ability to track whether a fixed property was set on startup
by recording the runtime state of whether a user had set the property
or not
* Update AccumuloConfigurationIsPropertySetTest to ensure that the
runtime properties behavior is working correctly
* Also, rename a scan server property enum to match the others
This fixes #3529
---
.../accumulo/core/conf/AccumuloConfiguration.java | 4 +++-
.../org/apache/accumulo/core/conf/Property.java | 6 +++--
.../server/conf/RuntimeFixedProperties.java | 13 ++++++++++
.../accumulo/server/conf/SystemConfiguration.java | 5 +++-
.../AccumuloConfigurationIsPropertySetTest.java | 28 ++++++++++++++++++----
.../server/conf/SystemConfigurationTest.java | 5 ++--
.../org/apache/accumulo/tserver/ScanServer.java | 2 +-
.../accumulo/test/ScanServerMetadataEntriesIT.java | 2 +-
8 files changed, 52 insertions(+), 13 deletions(-)
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 5ad65b371e..faba4facc7 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
@@ -432,7 +432,9 @@ public abstract class AccumuloConfiguration implements
Iterable<Entry<String,Str
/**
* @param prop Property to check
- * @return true if the given property has explicitly been set by a user,
false otherwise
+ * @return true if the given property has explicitly been set by a user,
false otherwise; for
+ * runtime-fixed properties, this returns true only if the property
was set by the user
+ * when the property's value was first read and entered a fixed state
*/
public abstract boolean isPropertySet(Property prop);
diff --git a/core/src/main/java/org/apache/accumulo/core/conf/Property.java
b/core/src/main/java/org/apache/accumulo/core/conf/Property.java
index 20f9849b65..445e693c60 100644
--- a/core/src/main/java/org/apache/accumulo/core/conf/Property.java
+++ b/core/src/main/java/org/apache/accumulo/core/conf/Property.java
@@ -502,7 +502,7 @@ public enum Property {
SSERV_SCAN_EXECUTORS_META_THREADS("sserver.scan.executors.meta.threads",
"8", PropertyType.COUNT,
"The number of threads for the metadata table scan executor.", "2.1.0"),
@Experimental
- SSERVER_SCAN_REFERENCE_EXPIRATION_TIME("sserver.scan.reference.expiration",
"5m",
+ SSERV_SCAN_REFERENCE_EXPIRATION_TIME("sserver.scan.reference.expiration",
"5m",
PropertyType.TIMEDURATION,
"The amount of time a scan reference is unused before its deleted from
metadata table.",
"2.1.0"),
@@ -1844,6 +1844,8 @@ public enum Property {
|| key.startsWith(TABLE_CRYPTO_PREFIX.getKey()));
}
+ // these properties are fixed to a specific value at startup and require a
restart for changes to
+ // take effect; these are always system-level properties, and not namespace
or table properties
public static final EnumSet<Property> fixedProperties = EnumSet.of(
// port options
GC_PORT, MANAGER_CLIENTPORT, TSERV_CLIENTPORT, SSERV_CLIENTPORT,
SSERV_PORTSEARCH,
@@ -1861,7 +1863,7 @@ public enum Property {
TSERV_DEFAULT_BLOCKSIZE, SSERV_DEFAULT_BLOCKSIZE,
// sserver specific options
- SSERVER_SCAN_REFERENCE_EXPIRATION_TIME,
SSERV_CACHED_TABLET_METADATA_EXPIRATION,
+ SSERV_SCAN_REFERENCE_EXPIRATION_TIME,
SSERV_CACHED_TABLET_METADATA_EXPIRATION,
// thread options
TSERV_MINTHREADS, TSERV_MINTHREADS_TIMEOUT, SSERV_MINTHREADS,
SSERV_MINTHREADS_TIMEOUT,
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 959402f0dc..4d56771324 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
@@ -22,7 +22,9 @@ import static java.util.Objects.requireNonNull;
import java.util.Collections;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.Map;
+import java.util.Set;
import org.apache.accumulo.core.conf.Property;
import org.apache.accumulo.core.conf.SiteConfiguration;
@@ -52,6 +54,9 @@ public class RuntimeFixedProperties {
// original, stored fixed props.
private final Map<String,String> origStored = new HashMap<>();
+ // the set of properties that were set by the user when they were fixed
+ private final Set<Property> wasSetByUser = new HashSet<>();
+
public RuntimeFixedProperties(final Map<String,String> storedProps,
final SiteConfiguration siteConfig) {
requireNonNull(siteConfig, "a site configuration must be provided");
@@ -62,9 +67,13 @@ public class RuntimeFixedProperties {
// use value in ZooKeeper
if (value != null) {
origStored.put(key, value);
+ wasSetByUser.add(prop);
} else {
// Not in ZK, use config or default.
value = siteConfig.get(prop);
+ if (siteConfig.isPropertySet(prop)) {
+ wasSetByUser.add(prop);
+ }
}
fixed.put(key, value);
log.trace("fixed property name: {} = {}", key, value);
@@ -76,6 +85,10 @@ public class RuntimeFixedProperties {
return fixed.get(property.getKey());
}
+ public boolean wasPropertySet(final Property property) {
+ return wasSetByUser.contains(property);
+ }
+
@VisibleForTesting
Map<String,String> getAll() {
return Collections.unmodifiableMap(fixed);
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/conf/SystemConfiguration.java
b/server/base/src/main/java/org/apache/accumulo/server/conf/SystemConfiguration.java
index d4b79aed28..809f23c306 100644
---
a/server/base/src/main/java/org/apache/accumulo/server/conf/SystemConfiguration.java
+++
b/server/base/src/main/java/org/apache/accumulo/server/conf/SystemConfiguration.java
@@ -62,6 +62,9 @@ public class SystemConfiguration extends
ZooBasedConfiguration {
@Override
public boolean isPropertySet(Property prop) {
- return runtimeFixedProps.get(prop) != null || super.isPropertySet(prop);
+ if (Property.isFixedZooPropertyKey(prop)) {
+ return runtimeFixedProps.wasPropertySet(prop);
+ }
+ return super.isPropertySet(prop);
}
}
diff --git
a/server/base/src/test/java/org/apache/accumulo/server/conf/AccumuloConfigurationIsPropertySetTest.java
b/server/base/src/test/java/org/apache/accumulo/server/conf/AccumuloConfigurationIsPropertySetTest.java
index fb7dbb9542..3097ab6994 100644
---
a/server/base/src/test/java/org/apache/accumulo/server/conf/AccumuloConfigurationIsPropertySetTest.java
+++
b/server/base/src/test/java/org/apache/accumulo/server/conf/AccumuloConfigurationIsPropertySetTest.java
@@ -37,6 +37,7 @@ import static org.easymock.EasyMock.expect;
import static org.easymock.EasyMock.expectLastCall;
import static org.easymock.EasyMock.replay;
import static org.easymock.EasyMock.verify;
+import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -226,12 +227,29 @@ public class AccumuloConfigurationIsPropertySetTest
extends WithTestNames {
var conf = new SystemConfiguration(context, sysPropKey, parent);
verifyIsSet(conf, shouldBeSet, shouldNotBeSet, inGetProperties(conf));
- // these get added from the constructor via RuntimeFixedProperties and get
checked in the
- // isPropertySet impl; adding these to the expected list is a workaround
until
- // https://github.com/apache/accumulo/issues/3529 is fixed
- shouldBeSet.addAll(Property.fixedProperties);
+ // also verify using isPropertySet
+ verifyIsSet(conf, shouldBeSet, shouldNotBeSet, conf::isPropertySet);
- // verify using isPropertySet
+ // now set a few fixed properties on the parent to simulate a user setting
a fixed property that
+ // requires a restart
+ var shouldBeSetMore = new HashSet<Property>(shouldBeSet);
+ Property.fixedProperties.stream().limit(5).forEach(p -> {
+ // use the default value so we can verify it's actually set, and not
just looks set because it
+ // has the same value as the default
+ parent.set(p.getKey(), p.getDefaultValue());
+ shouldBeSetMore.add(p);
+ });
+ // make sure we actually added some
+ assertEquals(5, Sets.symmetricDifference(shouldBeSet,
shouldBeSetMore).size());
+
+ // verify that the view of the configuration now includes the fixed
properties, because we added
+ // them to the parent; however, in a real system, the parent is the
SiteConfiguration, which is
+ // immutable, and this isn't possible; so this is just an easy way to
verify that the values
+ // were actually altered (easier than mocking user edits to ZooKeeper)
+ var shouldNotBeSetMore = Sets.difference(ALL_PROPERTIES, shouldBeSetMore);
+ verifyIsSet(conf, shouldBeSetMore, shouldNotBeSetMore,
inGetProperties(conf));
+ // now, verify that the configuration view is unchanged when looking at
isPropertySet, because
+ // that is respecting the fact that these fixed properties were not set at
startup
verifyIsSet(conf, shouldBeSet, shouldNotBeSet, conf::isPropertySet);
}
diff --git
a/server/base/src/test/java/org/apache/accumulo/server/conf/SystemConfigurationTest.java
b/server/base/src/test/java/org/apache/accumulo/server/conf/SystemConfigurationTest.java
index 34ed91f88a..17a8a561fa 100644
---
a/server/base/src/test/java/org/apache/accumulo/server/conf/SystemConfigurationTest.java
+++
b/server/base/src/test/java/org/apache/accumulo/server/conf/SystemConfigurationTest.java
@@ -32,6 +32,7 @@ import static org.easymock.EasyMock.expectLastCall;
import static org.easymock.EasyMock.replay;
import static org.easymock.EasyMock.reset;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.time.Instant;
@@ -104,7 +105,7 @@ public class SystemConfigurationTest {
assertEquals("true", sysConfig.get(TABLE_BLOOM_ENABLED)); // sys config
assertEquals(TABLE_BLOOM_SIZE.getDefaultValue(),
sysConfig.get(TABLE_BLOOM_SIZE)); // default
- assertTrue(sysConfig.isPropertySet(TSERV_CLIENTPORT)); // default
+ assertFalse(sysConfig.isPropertySet(TSERV_CLIENTPORT)); // default
assertTrue(sysConfig.isPropertySet(GC_PORT)); // fixed sys config
assertTrue(sysConfig.isPropertySet(TSERV_SCAN_MAX_OPENFILES)); // fixed
sys config
assertTrue(sysConfig.isPropertySet(TABLE_BLOOM_ENABLED)); // sys config
@@ -126,7 +127,7 @@ public class SystemConfigurationTest {
assertEquals("false", sysConfig.get(TABLE_BLOOM_ENABLED)); // sys config
assertEquals("2048", sysConfig.get(TABLE_BLOOM_SIZE)); // default
- assertTrue(sysConfig.isPropertySet(TSERV_CLIENTPORT)); // default
+ assertFalse(sysConfig.isPropertySet(TSERV_CLIENTPORT)); // default
assertTrue(sysConfig.isPropertySet(GC_PORT)); // fixed sys config
assertTrue(sysConfig.isPropertySet(TSERV_SCAN_MAX_OPENFILES)); // fixed
sys config
assertTrue(sysConfig.isPropertySet(TABLE_BLOOM_ENABLED)); // sys config
diff --git
a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java
b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java
index 38672f76ec..f959d2a010 100644
--- a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java
+++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java
@@ -237,7 +237,7 @@ public class ScanServer extends AbstractServer
getConfiguration().getTimeInMillis(Property.SSERV_CACHED_TABLET_METADATA_EXPIRATION);
long scanServerReservationExpiration =
-
getConfiguration().getTimeInMillis(Property.SSERVER_SCAN_REFERENCE_EXPIRATION_TIME);
+
getConfiguration().getTimeInMillis(Property.SSERV_SCAN_REFERENCE_EXPIRATION_TIME);
tabletMetadataLoader = new TabletMetadataLoader(getContext().getAmple());
diff --git
a/test/src/main/java/org/apache/accumulo/test/ScanServerMetadataEntriesIT.java
b/test/src/main/java/org/apache/accumulo/test/ScanServerMetadataEntriesIT.java
index e1c505ea72..2c026ef66e 100644
---
a/test/src/main/java/org/apache/accumulo/test/ScanServerMetadataEntriesIT.java
+++
b/test/src/main/java/org/apache/accumulo/test/ScanServerMetadataEntriesIT.java
@@ -80,7 +80,7 @@ public class ScanServerMetadataEntriesIT extends
SharedMiniClusterBase {
org.apache.hadoop.conf.Configuration coreSite) {
cfg.setNumScanServers(1);
cfg.setProperty(Property.TSERV_SESSION_MAXIDLE, "3s");
- cfg.setProperty(Property.SSERVER_SCAN_REFERENCE_EXPIRATION_TIME, "5s");
+ cfg.setProperty(Property.SSERV_SCAN_REFERENCE_EXPIRATION_TIME, "5s");
}
}
