This is an automated email from the ASF dual-hosted git repository.
edcoleman 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 df7e49e Refactor common table property validation to reduce
duplication (#2186)
df7e49e is described below
commit df7e49e771683f73d228ca02af0c40f475ac90e0
Author: EdColeman <d...@etcoleman.com>
AuthorDate: Wed Jul 7 14:10:56 2021 -0400
Refactor common table property validation to reduce duplication (#2186)
* Refactor common table property validation to reduce duplication
- Move duplcated code for table property validation to one place
- Minor check-style fixes.
- fix possible nulll pointer warning (FateServiceHandler)
---
.../org/apache/accumulo/core/conf/Property.java | 44 +++++++++++++---------
.../accumulo/server/util/NamespacePropUtil.java | 8 +---
.../apache/accumulo/server/util/TablePropUtil.java | 8 +---
.../accumulo/manager/FateServiceHandler.java | 8 ++--
4 files changed, 33 insertions(+), 35 deletions(-)
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 74f22a5..3093990 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
@@ -1171,15 +1171,15 @@ public enum Property {
"This property is deprecated since 2.0.0, use
tserver.scan.executors.meta.threads instead. "
+ "The maximum number of concurrent metadata read ahead that will
execute.");
- private String key;
- private String defaultValue;
- private String description;
+ private final String key;
+ private final String defaultValue;
+ private final String description;
private boolean annotationsComputed = false;
private boolean isSensitive;
private boolean isDeprecated;
private boolean isExperimental;
private Property replacedBy = null;
- private PropertyType type;
+ private final PropertyType type;
Property(String name, String defaultValue, PropertyType type, String
description) {
this.key = name;
@@ -1352,10 +1352,10 @@ public enum Property {
return false;
}
- private static HashSet<String> validTableProperties = null;
- private static HashSet<String> validProperties = null;
- private static HashSet<String> validPrefixes = null;
- private static HashMap<String,Property> propertiesByKey = null;
+ private static final HashSet<String> validTableProperties = new HashSet<>();
+ private static final HashSet<String> validProperties = new HashSet<>();
+ private static final HashSet<String> validPrefixes = new HashSet<>();
+ private static final HashMap<String,Property> propertiesByKey = new
HashMap<>();
private static boolean isKeyValidlyPrefixed(String key) {
for (String prefix : validPrefixes) {
@@ -1368,6 +1368,22 @@ public enum Property {
}
/**
+ * Checks if the given property and value are valid. A property is valid if
the property key is
+ * valid see {@link #isValidTablePropertyKey} and that the value is a valid
format for the type
+ * see {@link PropertyType#isValidFormat}.
+ *
+ * @param key
+ * property key
+ * @param value
+ * property value
+ * @return true if key is valid (recognized, or has a recognized prefix)
+ */
+ public static boolean isTablePropertyValid(final String key, final String
value) {
+ Property p = getPropertyByKey(key);
+ return (p == null || p.getType().isValidFormat(value)) &&
isValidTablePropertyKey(key);
+ }
+
+ /**
* Checks if the given property key is valid. A valid property key is either
equal to the key of
* some defined property or has a prefix matching some prefix defined in
this class.
*
@@ -1528,21 +1544,15 @@ public enum Property {
// Precomputing information here avoids :
// * Computing it each time a method is called
// * Using synch to compute the first time a method is called
- propertiesByKey = new HashMap<>();
- validPrefixes = new HashSet<>();
- validProperties = new HashSet<>();
-
for (Property p : Property.values()) {
+ propertiesByKey.put(p.getKey(), p);
if (p.getType().equals(PropertyType.PREFIX)) {
validPrefixes.add(p.getKey());
} else {
validProperties.add(p.getKey());
}
- propertiesByKey.put(p.getKey(), p);
- }
-
- validTableProperties = new HashSet<>();
- for (Property p : Property.values()) {
+ // exclude prefix types (prevents setting a prefix type like
table.custom or
+ // table.constraint, directly, since they aren't valid properties on
their own)
if (!p.getType().equals(PropertyType.PREFIX)
&& p.getKey().startsWith(Property.TABLE_PREFIX.getKey())) {
validTableProperties.add(p.getKey());
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/util/NamespacePropUtil.java
b/server/base/src/main/java/org/apache/accumulo/server/util/NamespacePropUtil.java
index 94cc565..bb809c6 100644
---
a/server/base/src/main/java/org/apache/accumulo/server/util/NamespacePropUtil.java
+++
b/server/base/src/main/java/org/apache/accumulo/server/util/NamespacePropUtil.java
@@ -32,7 +32,7 @@ import org.apache.zookeeper.KeeperException;
public class NamespacePropUtil {
public static boolean setNamespaceProperty(ServerContext context,
NamespaceId namespaceId,
String property, String value) throws KeeperException,
InterruptedException {
- if (!isPropertyValid(property, value))
+ if (!Property.isTablePropertyValid(property, value))
return false;
ZooReaderWriter zoo = context.getZooReaderWriter();
@@ -49,12 +49,6 @@ public class NamespacePropUtil {
return true;
}
- public static boolean isPropertyValid(String property, String value) {
- Property p = Property.getPropertyByKey(property);
- return (p == null || p.getType().isValidFormat(value))
- && Property.isValidTablePropertyKey(property);
- }
-
public static void removeNamespaceProperty(ServerContext context,
NamespaceId namespaceId,
String property) throws InterruptedException, KeeperException {
String zPath = getPath(context, namespaceId) + "/" + property;
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/util/TablePropUtil.java
b/server/base/src/main/java/org/apache/accumulo/server/util/TablePropUtil.java
index 90e1b97..8980c6b 100644
---
a/server/base/src/main/java/org/apache/accumulo/server/util/TablePropUtil.java
+++
b/server/base/src/main/java/org/apache/accumulo/server/util/TablePropUtil.java
@@ -39,7 +39,7 @@ public class TablePropUtil {
public static boolean setTableProperty(ZooReaderWriter zoo, String zkRoot,
TableId tableId,
String property, String value) throws KeeperException,
InterruptedException {
- if (!isPropertyValid(property, value))
+ if (!Property.isTablePropertyValid(property, value))
return false;
// create the zk node for per-table properties for this table if it
doesn't already exist
@@ -53,12 +53,6 @@ public class TablePropUtil {
return true;
}
- public static boolean isPropertyValid(String property, String value) {
- Property p = Property.getPropertyByKey(property);
- return (p == null || p.getType().isValidFormat(value))
- && Property.isValidTablePropertyKey(property);
- }
-
public static void removeTableProperty(ServerContext context, TableId
tableId, String property)
throws InterruptedException, KeeperException {
String zPath = getTablePath(context.getZooKeeperRoot(), tableId) + "/" +
property;
diff --git
a/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java
b/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java
index 0be11ad..a657b90 100644
---
a/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java
+++
b/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java
@@ -57,6 +57,7 @@ import
org.apache.accumulo.core.clientImpl.thrift.TableOperation;
import org.apache.accumulo.core.clientImpl.thrift.TableOperationExceptionType;
import org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException;
import
org.apache.accumulo.core.clientImpl.thrift.ThriftTableOperationException;
+import org.apache.accumulo.core.conf.Property;
import org.apache.accumulo.core.data.NamespaceId;
import org.apache.accumulo.core.data.TableId;
import org.apache.accumulo.core.manager.thrift.FateOperation;
@@ -85,7 +86,6 @@ import
org.apache.accumulo.manager.tableOps.tableExport.ExportTable;
import org.apache.accumulo.manager.tableOps.tableImport.ImportTable;
import org.apache.accumulo.server.client.ClientServiceHandler;
import org.apache.accumulo.server.manager.state.MergeInfo;
-import org.apache.accumulo.server.util.TablePropUtil;
import org.apache.hadoop.fs.FSDataOutputStream;
import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.Path;
@@ -286,7 +286,7 @@ class FateServiceHandler implements FateService.Iface {
continue;
}
- if (!TablePropUtil.isPropertyValid(entry.getKey(),
entry.getValue())) {
+ if (!Property.isTablePropertyValid(entry.getKey(),
entry.getValue())) {
throw new ThriftTableOperationException(null, tableName, tableOp,
TableOperationExceptionType.OTHER,
"Property or value not valid " + entry.getKey() + "=" +
entry.getValue());
@@ -694,8 +694,8 @@ class FateServiceHandler implements FateService.Iface {
// Information provided by a client should generate a user-level
exception, not a system-level
// warning.
log.debug(why);
- throw new ThriftTableOperationException(tableId.canonical(), null, op,
- TableOperationExceptionType.INVALID_NAME, why);
+ throw new ThriftTableOperationException((tableId == null ? "null" :
tableId.canonical()),
+ null, op, TableOperationExceptionType.INVALID_NAME, why);
}
}
