[GitHub] [accumulo] ctubbsii commented on a change in pull request #2467: Added mechanism to cancel FaTE operations from Shell with Manager up

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

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



##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ManagerClient.java
##########
@@ -193,4 +195,32 @@ public static void executeVoid(ClientContext context,
       throw new AssertionError(e);
     }
   }
+
+  public static boolean cancelFateOperation(ClientContext context, Long txid)
+      throws AccumuloException, AccumuloSecurityException {
+    ManagerClientService.Client client = null;
+    while (true) {

Review comment:
       Alternatively, put this null assignment inside the while loop to clear it from previous iterations.
   ```suggestion
       while (true) {
         ManagerClientService.Client client = null;
   ```

##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ManagerClient.java
##########
@@ -193,4 +195,32 @@ public static void executeVoid(ClientContext context,
       throw new AssertionError(e);
     }
   }
+
+  public static boolean cancelFateOperation(ClientContext context, Long txid)
+      throws AccumuloException, AccumuloSecurityException {
+    ManagerClientService.Client client = null;
+    while (true) {
+      try {
+        client = getConnectionWithRetry(context);
+        return client.cancelFateOperation(TraceUtil.traceInfo(), context.rpcCreds(), txid);
+      } catch (TTransportException tte) {
+        log.debug("ManagerClient request failed, retrying ... ", tte);
+        sleepUninterruptibly(100, TimeUnit.MILLISECONDS);
+      } catch (ThriftSecurityException e) {
+        throw new AccumuloSecurityException(e.user, e.code, e);
+      } catch (ThriftTableOperationException e) {
+        throw new AccumuloException(e);
+      } catch (ThriftNotActiveServiceException e) {
+        // Let it loop, fetching a new location
+        log.debug("Contacted a Manager which is no longer active, re-creating"
+            + " the connection to the active Manager");
+      } catch (Exception e) {
+        throw new AccumuloException(e);
+      } finally {
+        if (client != null)
+          close(client, context);

Review comment:
       Since client is re-assigned in each iteration of the loop, it'd be good to set it null here, in case the assignment fails, we don't try to re-close the value from the previous iteration.
   ```suggestion
           if (client != null) {
             close(client, context);
           }
           client = null;
   ```




-- 
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-unsubscribe@accumulo.apache.org

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