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

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

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



##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
    */
   void waitForBalance() throws AccumuloException;
 
+  /**
+   * Throws an exception if a tablet server can not be contacted.
+   *
+   * @param args
+   *          Command line arguments passed in from user command.
+   * @param secretOption
+   *          Specified instance secret to use for commands.
+   *
+   * @since 2.1.0
+   */
+  boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
+
+  /**
+   * Throws an exception if a tablet server can not be contacted.
+   *
+   * @param args
+   *          Command line arguments passed in from user command.
+   * @param secretOption
+   *          Specified instance secret to use for commands.
+   *
+   * @since 2.1.0
+   */
+  boolean fateDelete(List<String> args, String secretOption) throws AccumuloException;
+
+  /**
+   * Throws an exception if a tablet server can not be contacted.
+   *
+   * @param args
+   *          Command line arguments passed in from user command.
+   * @param filterTxid
+   *          Parsed transaction IDs for print filter.
+   * @param filterStatus
+   *          Parsed TStatus for print filter.
+   * @param secretOption
+   *          Specified instance secret to use for commands.
+   *
+   * @since 2.1.0
+   */
+  String fatePrint(List<String> args, Set<Long> filterTxid, EnumSet<TStatus> filterStatus,

Review comment:
       > > Instead of returning "String" with "Print" in the method name, this could return a map of fate IDs to statuses, or some similar data structure.
   > 
   > Is there a benefit you had in mind with this possible change?
   
   Well, mainly just that APIs should return objects that can be manipulated/read/transformed by calling code. It's not a console REPL where we have only STDIN/STDOUT and have to parse STDOUT from a command. We can have actual typed data. So, there's no reason to resort to "String" types to represent complex information in the API.
   
   > Currently, the returning string is the entire output from `Admin.print` that then gets printed to the shell. Admin print handles all the formatting, the only thing FateCommand has to do is print it to the shell.
   
   Right, that's all that the shell needs to do... but if we're going through the effort of creating a public API endpoint, we're not doing it just for the shell. The shell can take a complex object returned from the public API and format it into a string for printing. But other callers to this API endpoint might want to do something different with the data. When creating a new public API endpoint, we should try to consider the value it has to a generic caller of the public API, and not just the single use case that inspired its creation.
   
   > Instead of 'Print', I could change the name to "fateStatus" as I think that more accurately describes the intended use of the function. It shows the status of either all the txid's in operation or the specific one the user requests.
   
   The name is fine. It could return something that looks like `Set<FateStatus>` or `Map<Txid,Status>` or similar.




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