From commits-return-25060-archive-asf-public=cust-asf.ponee.io@accumulo.apache.org Wed Jul 7 18:11:06 2021 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mxout1-ec2-va.apache.org (mxout1-ec2-va.apache.org [3.227.148.255]) by mx-eu-01.ponee.io (Postfix) with ESMTPS id 459B418066B for ; Wed, 7 Jul 2021 20:11:06 +0200 (CEST) Received: from mail.apache.org (mailroute1-lw-us.apache.org [207.244.88.153]) by mxout1-ec2-va.apache.org (ASF Mail Server at mxout1-ec2-va.apache.org) with SMTP id 7A4153F19C for ; Wed, 7 Jul 2021 18:11:05 +0000 (UTC) Received: (qmail 39461 invoked by uid 500); 7 Jul 2021 18:11:05 -0000 Mailing-List: contact commits-help@accumulo.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@accumulo.apache.org Delivered-To: mailing list commits@accumulo.apache.org Received: (qmail 39444 invoked by uid 99); 7 Jul 2021 18:11:04 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 07 Jul 2021 18:11:04 +0000 Received: by gitbox.apache.org (ASF Mail Server at gitbox.apache.org, from userid 33) id BDB888DC92; Wed, 7 Jul 2021 18:11:04 +0000 (UTC) Date: Wed, 07 Jul 2021 18:11:04 +0000 To: "commits@accumulo.apache.org" Subject: [accumulo] branch main updated: Refactor common table property validation to reduce duplication (#2186) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Message-ID: <162568146450.19463.3676670907109540968@gitbox.apache.org> From: edcoleman@apache.org X-Git-Host: gitbox.apache.org X-Git-Repo: accumulo X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Oldrev: 7bbe579bff33e6173237e44eeaeead71f502a3e5 X-Git-Newrev: df7e49e771683f73d228ca02af0c40f475ac90e0 X-Git-Rev: df7e49e771683f73d228ca02af0c40f475ac90e0 X-Git-NotificationType: ref_changed_plus_diff X-Git-Multimail-Version: 1.5.dev Auto-Submitted: auto-generated 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 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 validTableProperties = null; - private static HashSet validProperties = null; - private static HashSet validPrefixes = null; - private static HashMap propertiesByKey = null; + private static final HashSet validTableProperties = new HashSet<>(); + private static final HashSet validProperties = new HashSet<>(); + private static final HashSet validPrefixes = new HashSet<>(); + private static final HashMap 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); } }