[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2215: Remove the use of SiteConfiguration from FateCommand

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

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


##########
core/src/main/java/org/apache/accumulo/fate/AdminUtil.java:
##########
@@ -478,32 +411,32 @@ public boolean prepFail(TStore<T> zs, ZooReaderWriter zk, ServiceLockPath zLockM
     try {
       txid = Long.parseLong(txidStr, 16);
     } catch (NumberFormatException nfe) {
-      System.out.printf("Invalid transaction ID format: %s%n", txidStr);
+      log.error("Invalid transaction ID format: {}", txidStr);
       return false;
     }
     boolean state = false;
     zs.reserve(txid);
     TStatus ts = zs.getStatus(txid);
     switch (ts) {
       case UNKNOWN:
-        System.out.printf("Invalid transaction ID: %016x%n", txid);
+        log.error("Invalid transaction ID: {}", txid);

Review Comment:
   These are a bit of a change in behavior for the shell, which I think currently uses AdminUtil. I think this behavior is better, but might want to include some release notes that mention the change, so if anybody was relying on scripting the shell, and capturing STDOUT, it will work differently for them, depending on their log configuration.



##########
core/src/main/java/org/apache/accumulo/core/clientImpl/InstanceOperationsImpl.java:
##########
@@ -284,6 +287,42 @@ public void waitForBalance() throws AccumuloException {
 
   }
 
+  @Override
+  public void fateFail(List<String> txids) throws AccumuloException {
+    checkArgument(txids != null, "txids is null");
+    executeAdminOperation(AdminOperation.FAIL, txids, null);
+  }
+
+  @Override
+  public void fateDelete(List<String> txids) throws AccumuloException {
+    checkArgument(txids != null, "txids is null");
+    executeAdminOperation(AdminOperation.DELETE, txids, null);
+  }
+
+  @Override
+  public List<TransactionStatus> fateStatus(List<String> txids, List<String> tStatus)
+      throws AccumuloException {
+    checkArgument(txids != null, "txids is null");
+    List<TransactionStatus> txStatus = new ArrayList<>();
+    for (var tx : executeAdminOperation(AdminOperation.PRINT, txids, tStatus)) {
+      txStatus.add(new TransactionStatus(tx.getTxid(), tx.getTstatus(), tx.getDebug(),
+          tx.getHlocks(), tx.getWlocks(), tx.getTop(), tx.getTimecreated(), tx.getStackInfo()));
+    }
+    return txStatus;
+  }
+
+  private List<FateTransaction> executeAdminOperation(AdminOperation op, List<String> txids,
+      List<String> filterStatuses) throws AccumuloException {
+    try {
+      return ThriftClientTypes.CLIENT.execute(context,
+          client -> client.executeAdminOperation(TraceUtil.traceInfo(), context.rpcCreds(), op,
+              txids, filterStatuses));
+    } catch (AccumuloSecurityException e) {
+      throw new RuntimeException("Unexpected exception thrown", e);

Review Comment:
   It seems like AccumuloSecurityException should propagate up.



##########
core/src/main/java/org/apache/accumulo/fate/AdminUtil.java:
##########
@@ -360,6 +285,10 @@ private FateStatus getTransactionStatus(ReadOnlyTStore<T> zs, Set<Long> filterTx
 
     List<Long> transactions = zs.list();
     List<TransactionStatus> statuses = new ArrayList<>(transactions.size());
+    Gson gson = new GsonBuilder()
+        .registerTypeAdapter(ReadOnlyRepo.class, new InterfaceSerializer<>())
+        .registerTypeAdapter(Repo.class, new InterfaceSerializer<>())
+        .registerTypeAdapter(byte[].class, new ByteArraySerializer()).setPrettyPrinting().create();

Review Comment:
   What are these type adapters doing? An inline comment could help.



##########
server/base/src/main/java/org/apache/accumulo/server/client/ClientServiceHandler.java:
##########
@@ -447,6 +460,83 @@ public List<TDiskUsage> getDiskUsage(Set<String> tables, TCredentials credential
     }
   }
 
+  @Override
+  public List<FateTransaction> executeAdminOperation(TInfo tInfo, TCredentials credentials,
+      AdminOperation op, List<String> txids, List<String> filterStatuses)
+      throws ThriftSecurityException, TException {
+    try {
+      authenticate(tInfo, credentials);

Review Comment:
   In addition to authenticating, we probably want to restrict this to users with admin permissions, right?



-- 
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