ctubbsii commented on code in PR #5334:
URL: https://github.com/apache/accumulo/pull/5334#discussion_r1955342089
##########
server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java:
##########
@@ -286,20 +286,26 @@ public void executeFateOperation(TInfo tinfo,
TCredentials c, long opid, FateOpe
keepOffline =
Boolean.parseBoolean(ByteBufferUtil.toString(arguments.get(2)));
}
+ NamespaceId srcNamespaceId;
NamespaceId namespaceId;
try {
+ srcNamespaceId = manager.getContext().getNamespaceId(srcTableId);
namespaceId = Namespaces.getNamespaceId(manager.getContext(),
TableNameUtil.qualify(tableName).getFirst());
} catch (NamespaceNotFoundException e) {
// shouldn't happen, but possible once cloning between namespaces is
supported
throw new ThriftTableOperationException(null, tableName, tableOp,
TableOperationExceptionType.NAMESPACE_NOTFOUND, "");
+ } catch (TableNotFoundException e) {
+ // shouldn't happen, but possible once cloning between namespaces is
supported
+ throw new ThriftTableOperationException(srcTableId.canonical(),
null, tableOp,
+ TableOperationExceptionType.NOTFOUND, "");
Review Comment:
This shouldn't be added to the existing try block, because it's specific to
the new line to look up the source namespaceId. It's not applicable to the
retrieval of the namespaceId for the destination table name.
Also, the comment in here isn't applicable to this exception. The reason
this shouldn't happen is because the source tableId should exist, and it did at
some point prior to this call, when we looked up the tableId from the name that
was passed in to the client request to clone. So, the proper comment should be
something like `// could happen if the table was deleted while processing this
request`
(Also, not related to this PR, but the comment about the
NamespaceNotFoundException saying "shouldn't happen" doesn't make sense because
cloning between namespaces is already supported.)
It also occurs to me that we need to make sure that you can't clone into the
built-in `accumulo` namespace. There might be a check for that elsewhere... I'm
not sure. But this should be tested.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
