ctubbsii commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r678564250
##########
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:
We wouldn't want TStatus in the public API, but this could be a set of
strings for the various statuses. The implementation can convert to the TStatus
type, if necessary.
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.
##########
File path: core/src/main/thrift/client.thrift
##########
@@ -331,6 +338,18 @@ service ClientService {
1:ThriftTableOperationException tope
)
+ string executeAdminOperation(
+ 3:trace.TInfo tinfo
+ 4:security.TCredentials credentials
+ 2:AdminOperation op
+ 5:list<string> arguments
+ 6:set<i64> filtertxids
+ 7:list<string> filterStatues
+ 8:string secretOption
Review comment:
For a new method that doesn't need to preserve backwards compatibility,
the parameters can be numbered in order starting at 1.
##########
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,
+ 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
+ */
+ String fateDump(List<String> args, String secretOption) throws
AccumuloException;
Review comment:
Not sure if we need both "Dump" and "Print" methods. They seem to be
doing similar things.
##########
File path: core/src/main/java/org/apache/accumulo/fate/AdminUtil.java
##########
@@ -533,26 +533,29 @@ public void deleteLocks(ZooReaderWriter zk, String path,
String txidStr)
}
}
+ // Follow on ticket to rework exception handling to
+ // coincide with changes to FateCommand. At the moment, user has no feedback
of why something
+ // failed without having to go check server logs.
@SuppressFBWarnings(value = "DM_EXIT",
justification = "TODO - should probably avoid System.exit here; "
+ "this code is used by the fate admin shell command")
public boolean checkGlobalLock(ZooReaderWriter zk, ServiceLockPath path) {
try {
if (ServiceLock.getLockData(zk.getZooKeeper(), path) != null) {
- System.err.println("ERROR: Manager lock is held, not running");
+ log.error("ERROR: Manager lock is held, not running");
Review comment:
If changing these to log messages, including "ERROR" in the message is
redundant.
##########
File path: core/src/main/java/org/apache/accumulo/fate/AdminUtil.java
##########
@@ -533,26 +533,29 @@ public void deleteLocks(ZooReaderWriter zk, String path,
String txidStr)
}
}
+ // Follow on ticket to rework exception handling to
+ // coincide with changes to FateCommand. At the moment, user has no feedback
of why something
+ // failed without having to go check server logs.
@SuppressFBWarnings(value = "DM_EXIT",
justification = "TODO - should probably avoid System.exit here; "
+ "this code is used by the fate admin shell command")
public boolean checkGlobalLock(ZooReaderWriter zk, ServiceLockPath path) {
try {
if (ServiceLock.getLockData(zk.getZooKeeper(), path) != null) {
- System.err.println("ERROR: Manager lock is held, not running");
+ log.error("ERROR: Manager lock is held, not running");
if (this.exitOnError)
System.exit(1);
else
return false;
}
} catch (KeeperException e) {
- System.err.println("ERROR: Could not read manager lock, not running " +
e.getMessage());
+ log.error("ERROR: Could not read manager lock, not running " +
e.getMessage());
if (this.exitOnError)
System.exit(1);
else
return false;
} catch (InterruptedException e) {
- System.err.println("ERROR: Could not read manager lock, not running" +
e.getMessage());
+ log.error("ERROR: Could not read manager lock, not running" +
e.getMessage());
Review comment:
To include the stack trace in the log message:
```suggestion
log.error("Could not read manager lock, not running", e);
```
##########
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;
Review comment:
I think I agree with @EdColeman 's observation that you can probably
just require these to be used with an authenticated user with Admin permission,
rather than provide the instance secret. The server side can use the instance
secret from its config file, without needing to pass it.
Not sure from this what values are appropriate for "args". Are these just a
list of FaTE transaction IDs to fail? The parameters to these methods could be
more specific.
--
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]
