You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@accumulo.apache.org by ct...@apache.org on 2022/06/01 18:04:53 UTC
[accumulo] branch main updated: Improve deprecated property resolution (#2730)
This is an automated email from the ASF dual-hosted git repository.
ctubbsii 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 56e0253a86 Improve deprecated property resolution (#2730)
56e0253a86 is described below
commit 56e0253a86534b662a47913785925baf489cb4ac
Author: Christopher Tubbs <ct...@apache.org>
AuthorDate: Wed Jun 1 14:04:48 2022 -0400
Improve deprecated property resolution (#2730)
* Improve deprecated property resolution
Resolve a sequence of deprecation changes, instead of just a single
deprecation with a replacement (for example, if property A is deprecated
and replaced with property B, then B is deprecated and replaced with C,
we'd resolve like:
```java
resolve(C, B, A);
```
rather than:
```java
resolve(C, resolve(B, A));
```
Also:
* To avoid errors, ensure that the current property isn't deprecated
* Check that the deprecated properties are actually deprecated
* Add a unit test case
* Update the javadoc
* Don't make method final (messes with EasyMock)
---
.../accumulo/core/conf/AccumuloConfiguration.java | 27 ++++++++++----
.../core/conf/AccumuloConfigurationTest.java | 41 ++++++++++++++++++++++
.../org/apache/accumulo/tserver/log/DfsLogger.java | 13 ++++---
.../org/apache/accumulo/tserver/tablet/Tablet.java | 6 ++--
4 files changed, 71 insertions(+), 16 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 17b48ab7b4..7652db531f 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
@@ -35,6 +35,7 @@ import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.function.Function;
import java.util.function.Predicate;
+import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;
@@ -94,14 +95,28 @@ public abstract class AccumuloConfiguration implements Iterable<Entry<String,Str
public abstract String get(Property property);
/**
- * Given a property and a deprecated property determine which one to use base on which one is set.
+ * Given a property that is not deprecated and an ordered list of deprecated properties, determine
+ * which one to use based on which is set by the user in the configuration. If the non-deprecated
+ * property is set, use that. Otherwise, use the first deprecated property that is set. If no
+ * deprecated properties are set, use the non-deprecated property by default, even though it is
+ * not set (since it is not set, it will resolve to its default value). Since the deprecated
+ * properties are checked in order, newer properties should be on the left, replacing older
+ * properties on the right, so if a newer property is set, it will be selected over any older
+ * property that may also be set.
*/
- public Property resolve(Property property, Property deprecatedProperty) {
- if (isPropertySet(property) || !isPropertySet(deprecatedProperty)) {
- return property;
- } else {
- return deprecatedProperty;
+ public Property resolve(Property property, Property... deprecated) {
+ if (property.isDeprecated()) {
+ throw new IllegalArgumentException("Unexpected deprecated " + property.name());
+ }
+ for (Property p : deprecated) {
+ if (!p.isDeprecated()) {
+ var notDeprecated = Stream.of(deprecated).filter(Predicate.not(Property::isDeprecated))
+ .map(Property::name).collect(Collectors.toList());
+ throw new IllegalArgumentException("Unexpected non-deprecated " + notDeprecated);
+ }
}
+ return isPropertySet(property) ? property
+ : Stream.of(deprecated).filter(this::isPropertySet).findFirst().orElse(property);
}
/**
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 5fb3cfa375..d0ba95db31 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
@@ -375,4 +375,45 @@ public class AccumuloConfigurationTest {
tc.getScanExecutors().stream().filter(c -> c.name.equals("hulksmash")).findFirst().get();
assertEquals(44, sec8.maxThreads);
}
+
+ // note: this is hard to test if there aren't any deprecated properties
+ // if that's the case, just comment this test out or create a dummy deprecated property
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testResolveDeprecated() {
+ var conf = new ConfigurationCopy();
+
+ // deprecated first argument
+ var e1 = assertThrows(IllegalArgumentException.class, () -> conf
+ .resolve(Property.INSTANCE_DFS_DIR, Property.INSTANCE_DFS_URI, Property.INSTANCE_DFS_URI));
+ assertEquals("Unexpected deprecated INSTANCE_DFS_DIR", e1.getMessage());
+
+ // non-deprecated second argument
+ var e2 = assertThrows(IllegalArgumentException.class,
+ () -> conf.resolve(Property.INSTANCE_VOLUMES, Property.INSTANCE_DFS_DIR,
+ Property.INSTANCE_SECRET, Property.INSTANCE_DFS_DIR, Property.INSTANCE_VOLUMES));
+ assertEquals("Unexpected non-deprecated [INSTANCE_SECRET, INSTANCE_VOLUMES]", e2.getMessage());
+
+ // empty second argument always resolves to non-deprecated first argument
+ assertSame(Property.INSTANCE_VOLUMES, conf.resolve(Property.INSTANCE_VOLUMES));
+
+ // none are set, resolve to non-deprecated
+ assertSame(Property.INSTANCE_VOLUMES, conf.resolve(Property.INSTANCE_VOLUMES,
+ Property.INSTANCE_DFS_DIR, Property.INSTANCE_DFS_URI));
+
+ // resolve to first deprecated argument that's set; here, it's the final one
+ conf.set(Property.INSTANCE_DFS_URI, "");
+ assertSame(Property.INSTANCE_DFS_URI, conf.resolve(Property.INSTANCE_VOLUMES,
+ Property.INSTANCE_DFS_DIR, Property.INSTANCE_DFS_URI));
+
+ // resolve to first deprecated argument that's set; now, it's the first one because both are set
+ conf.set(Property.INSTANCE_DFS_DIR, "");
+ assertSame(Property.INSTANCE_DFS_DIR, conf.resolve(Property.INSTANCE_VOLUMES,
+ Property.INSTANCE_DFS_DIR, Property.INSTANCE_DFS_URI));
+
+ // every property is set, so resolve to the non-deprecated one
+ conf.set(Property.INSTANCE_VOLUMES, "");
+ assertSame(Property.INSTANCE_VOLUMES, conf.resolve(Property.INSTANCE_VOLUMES,
+ Property.INSTANCE_DFS_DIR, Property.INSTANCE_DFS_URI));
+ }
}
diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java
index ac9ee8b878..e6c07cf0ee 100644
--- a/server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java
+++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java
@@ -273,9 +273,7 @@ public class DfsLogger implements Comparable<DfsLogger> {
}
@Override
- public void await() {
- return;
- }
+ public void await() {}
}
static final LoggerOperation NO_WAIT_LOGGER_OP = new NoWaitLoggerOperation();
@@ -469,12 +467,13 @@ public class DfsLogger implements Comparable<DfsLogger> {
log.debug("Got new write-ahead log: {}", this);
}
- @SuppressWarnings("deprecation")
static long getWalBlockSize(AccumuloConfiguration conf) {
long blockSize = conf.getAsBytes(Property.TSERV_WAL_BLOCKSIZE);
- if (blockSize == 0)
- blockSize = (long) (conf.getAsBytes(
- conf.resolve(Property.TSERV_WAL_MAX_SIZE, Property.TSERV_WALOG_MAX_SIZE)) * 1.1);
+ if (blockSize == 0) {
+ @SuppressWarnings("deprecation")
+ Property prop = conf.resolve(Property.TSERV_WAL_MAX_SIZE, Property.TSERV_WALOG_MAX_SIZE);
+ blockSize = (long) (conf.getAsBytes(prop) * 1.1);
+ }
return blockSize;
}
diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
index 027af6a408..6c6c82dd4d 100644
--- a/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
+++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
@@ -1705,9 +1705,9 @@ public class Tablet extends TabletBase {
// grab this outside of tablet lock.
@SuppressWarnings("deprecation")
- int maxLogs = tableConfiguration
- .getCount(tableConfiguration.resolve(Property.TSERV_WAL_MAX_REFERENCED, tableConfiguration
- .resolve(Property.TSERV_WALOG_MAX_REFERENCED, Property.TABLE_MINC_LOGS_MAX)));
+ Property prop = tableConfiguration.resolve(Property.TSERV_WAL_MAX_REFERENCED,
+ Property.TSERV_WALOG_MAX_REFERENCED, Property.TABLE_MINC_LOGS_MAX);
+ int maxLogs = tableConfiguration.getCount(prop);
String reason = null;
synchronized (this) {