Minor TableOperations changes #5510
Conversation
- Added early `return true` for all system tables for `TableOperations.exists()` - Move built in table check for `TableOperations.setTabletAvailability()` to be handled on server side instead of client side. Result is RPC now occurs for a built in table, and exception for calling `setTabletAvailability` on a built in table is changed from `IllegalArgumentException` to `AccumuloException`. All the other `TableOperations` that can't be called on a system table result in an `AccumuloException` if they are called on a system table, so this change keeps consistency with the other `TableOperations` - This commit additionally made it so that, for all `TableOperations`, on the client side, the only validation done on a table is checking whether the argument is formatted correctly (via Validators.EXISTING_TABLE_NAME and Validators.NEW_TABLE_NAME) (e.g., table name is not blank, not too long, etc.).
|
I may add more commits to this PR based on discussions in #5474. |
| @@ -2243,7 +2241,6 @@ private void validatePropertiesToSet(Map<String,String> opts, Map<String,String> | |||
| public void setTabletAvailability(String tableName, Range range, TabletAvailability availability) | |||
| throws AccumuloSecurityException, AccumuloException { | |||
| EXISTING_TABLE_NAME.validate(tableName); | |||
| NOT_BUILTIN_TABLE.validate(tableName); | |||
There was a problem hiding this comment.
I think we should leave this in, but add the check in FateServiceHandler. No reason for the RPC to the Manager when we know it's going to fail.
There was a problem hiding this comment.
In that case we get an IllegalArgumentException for a built in table here instead of a AccumuloException. I agree that there is no need for the RPC beforehand, but this is the pattern taken for all the other methods:
IllegalArgumentException - The table name doesn't make sense. Not expected format
AccumuloException - The table operation cannot be performed on that table
And was probably the original intention with #1953
and I think it should be consistent across all operations. If we want to keep built in table here, we should have it everywhere applicable on client-side
There was a problem hiding this comment.
I think IllegalArgumentException would also make sense for system tables with the added benefit of avoiding RPC, we just need to decide what is the behavior across all table operations
There was a problem hiding this comment.
Seems like we have two options, make it consistent everywhere, or keep past behavior. I don't think we need to remove the check to keep past behavior, we can just catch the IllegalArgumentException and raise an AccumuloException instead.
There was a problem hiding this comment.
I don't think we would want to catch a IAE then immediately throw AE throughout TableOperations
I would prefer to keep it consistent everywhere. This single method is the only one not consistent with the rest, and I don't think it was intentional.
If we want to go with checking for BUILTIN_TABLE, METADATA_TABLE, or ROOT_TABLE on the client side, maybe that could be it's own PR
There was a problem hiding this comment.
So, maybe a compromise would be to remove NOT_BUILTIN_TABLE, but add:
if (SystemTables.tableNames().contains(tableName) {
throw new AccumuloException("Cannot set table availability on a system table");
}
This is similar to what is happening here
There was a problem hiding this comment.
The code you linked should have been looking at all system tables (or none and just let it be handled server side), so fixed that in c564e58
Also added the suggested change.
…bility client-side
|
@kevinrr888 This was approved 2 weeks ago, but hasn't yet been merged. Is something blocking this from being able to be merged? |
return truefor all system tables forTableOperations.exists()TableOperations.setTabletAvailability()to be handled on server side instead of client side. Result is RPC now occurs for a built in table, and exception for callingsetTabletAvailabilityon a built in table is changed fromIllegalArgumentExceptiontoAccumuloException. All the otherTableOperationsthat can't be called on a system table result in anAccumuloExceptionif they are called on a system table, so this change keeps consistency with the otherTableOperationsTableOperations, the only validation done on the client side for a table is checking whether the argument is formatted correctly (via Validators.EXISTING_TABLE_NAME and Validators.NEW_TABLE_NAME) (e.g., table name is not blank, not too long, etc.). I believe this was the originally intention with Throw IllegalArgumentException in public API when table names aren't valid #1953I think this behavior of:
IllegalArgumentException- The table name doesn't make sense. Not expected formatAccumuloException- The table operation cannot be performed on that tableMakes sense, and is now what occurs for applicable
TableOperations. However, could argue thatIllegalArgumentExceptionmakes sense for passing a system table, we should just be consistent across allTableOperations