ctubbsii commented on code in PR #5443:
URL: https://github.com/apache/accumulo/pull/5443#discussion_r2023527081


##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/namespace/delete/DeleteNamespace.java:
##########
@@ -41,7 +49,26 @@ public long isReady(long id, Manager environment) throws 
Exception {
   }
 
   @Override
-  public Repo<Manager> call(long tid, Manager environment) {
+  public Repo<Manager> call(long tid, Manager environment)
+      throws AcceptableThriftTableOperationException {
+    try {
+      // Namespaces.getTableIds(..) uses the following cache, clear the cache 
to force
+      // Namespaces.getTableIds(..) to read from zookeeper.
+      environment.getContext().clearTableListCache();
+      // Since we have a write lock on the namespace id and all fate table 
operations get a read
+      // lock on the namespace id there is no need to worry about a fate 
operation concurrently
+      // changing table ids in this namespace.
+      List<TableId> tableIdsInNamespace =
+          Namespaces.getTableIds(environment.getContext(), namespaceId);
+      if (!tableIdsInNamespace.isEmpty()) {
+        throw new AcceptableThriftTableOperationException(null, null, 
TableOperation.DELETE,
+            TableOperationExceptionType.OTHER,

Review Comment:
   Could we add a different TableOperationExceptionType instead of relying on 
OTHER?



##########
core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsImpl.java:
##########
@@ -158,6 +163,12 @@ public void delete(String namespace) throws 
AccumuloException, AccumuloSecurityE
 
     try {
       doNamespaceFateOperation(FateOperation.NAMESPACE_DELETE, args, opts, 
namespace);
+    } catch (AccumuloException ae) {
+      if (ae.getMessage().contains(TABLES_EXISTS_IN_NAMESPACE_INDICATOR)) {
+        throw new NamespaceNotEmptyException(namespaceId.canonical(), 
namespace, null, ae);
+      } else {
+        throw ae;
+      }
     } catch (NamespaceExistsException e) {
       // should not happen
       throw new AssertionError(e);

Review Comment:
   Instead of relying on the special String to detect this situation, you could 
instead use this existing NamespaceExistsException as a special case with the 
semantics of "namespace still exists (because it wasn't empty and couldn't be 
deleted)".



-- 
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: notifications-unsubscr...@accumulo.apache.org

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

Reply via email to