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



##########
File path: core/src/main/thrift/client.thrift
##########
@@ -331,6 +338,16 @@ service ClientService {
     1:ThriftTableOperationException tope
   )
 
+  string executeAdminOperation(
+      2:trace.TInfo tinfo

Review comment:
       This can be numbered starting at 1. The numbers for the throws 
exceptions can also start at 1. They are a separate context, and it doesn't 
matter if they reuse numbers across contexts, as long as the numbers aren't 
reused within the same parameter/exception context.

##########
File path: 
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +150,46 @@ boolean testClassLoad(final String className, final String 
asTypeName)
    */
   void waitForBalance() throws AccumuloException;
 
+  /**
+   * Throws an exception if a tablet server can not be contacted.

Review comment:
       These javadocs should say what the method does normally. The explanation 
for when an exception is thrown can be in a javadoc `@throws SomeException when 
<explanation here>` line.

##########
File path: 
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +150,46 @@ boolean testClassLoad(final String className, final String 
asTypeName)
    */
   void waitForBalance() throws AccumuloException;
 
+  /**
+   * Throws an exception if a tablet server can not be contacted.
+   *
+   * @param txids
+   *          Transaction IDs.
+   * @since 2.1.0
+   */
+  void fateFail(List<String> txids) throws AccumuloException;
+
+  /**
+   * Throws an exception if a tablet server can not be contacted.
+   *
+   * @param txids
+   *          Transaction IDs.
+   * @since 2.1.0
+   */
+  void fateDelete(List<String> txids) throws AccumuloException;
+
+  /**
+   * Throws an exception if a tablet server can not be contacted.
+   *
+   * @param txids
+   *          Transaction IDs.
+   * @param tStatus
+   *          Parsed TStatus for print filter.
+   * @return String containing the output to print to the shell.
+   * @since 2.1.0
+   */
+  String fatePrint(List<String> txids, List<String> tStatus) throws 
AccumuloException;
+
+  /**
+   * Throws an exception if a tablet server can not be contacted.
+   *
+   * @param txids
+   *          Transaction IDs.
+   * @return String containing the output to print to the shell.
+   * @since 2.1.0
+   */
+  String fateDump(List<String> txids) throws AccumuloException;

Review comment:
       See previous review conversation for discussion about consolidating 
these, and having a meaningful return type other than String. These APIs should 
not be written exclusively for use with the Shell, but should have meaningful 
return types of their own, and not just formatted strings for the shell.




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


Reply via email to