[GitHub] [accumulo] ctubbsii commented on a change in pull request #1971: Throw IllegalArgumentException in Public API

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #1971:
URL: https://github.com/apache/accumulo/pull/1971#discussion_r646932615



##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java
##########
@@ -792,8 +829,14 @@ public void clone(String srcTableName, String newTableName, CloneConfiguration c
   @Override
   public void rename(String oldTableName, String newTableName) throws AccumuloSecurityException,
       TableNotFoundException, AccumuloException, TableExistsException {
-    checkArgument(newTableName.length() <= MAX_TABLE_NAME_LEN,
-        "Table name is longer than " + MAX_TABLE_NAME_LEN + " characters");
+    checkArgument(oldTableName.matches(VALID_TABLENAME_REGEX),
+        "oldTableName must only contain word characters (letters, digits, and underscores)"
+            + " and cannot exceed 1024 characters");

Review comment:
       Old names, before we enforced the length can still be longer 1024; It'd be important to allow these to be longer, so the rename can actually happen. However, I will include this for now, and include this fix in a follow-up.

##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java
##########
@@ -814,6 +861,9 @@ public void flush(String tableName) throws AccumuloException, AccumuloSecurityEx
   public void flush(String tableName, Text start, Text end, boolean wait)
       throws AccumuloException, AccumuloSecurityException, TableNotFoundException {
     checkArgument(tableName != null, "tableName is null");
+    checkArgument(tableName.matches(VALID_TABLENAME_REGEX),
+        "tableName must only contain word characters (letters, digits, and underscores)"
+            + " and cannot exceed 1024 characters");

Review comment:
       There is a lot of repetition here. It seems to me that the TableValidators class, if it were in the core jar, could validate non-nullness and the rest, with an appropriate message. However, this is a good start, so I will do a follow-up change to move the TableValidators into the core jar and leverage it to shorten these.

##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java
##########
@@ -188,6 +189,10 @@ public TableOperationsImpl(ClientContext context) {
   @Override
   public boolean exists(String tableName) {
     checkArgument(tableName != null, "tableName is null");
+    checkArgument(tableName.matches(VALID_TABLENAME_REGEX),
+        "tableName must only contain word characters (letters, digits, and underscores)"
+            + " and cannot exceed 1024 characters");

Review comment:
       Unfortunately, existing names could be longer than 1024, from before we added the limit. So, we will need to account for that. I will address this in a follow-up, so this can go ahead and be merged and closed out.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org