Skip to content

Commit

Permalink
Remove direct use of System.out in shell (apache#4154)
Browse files Browse the repository at this point in the history
* Show shell config load info as a log message, so it can be suppressed
  from output more easily (depending on logging config)
* Throw command line parse exception when an invalid shell option is
  used in FateCommand, instead of merely printing to System.out and
  returning
* Use shellState.getWriter() for command-output messages instead of
  hard-coded System.out (use System.out for related unit tests)
  • Loading branch information
ctubbsii authored Jan 23, 2024
1 parent c0d548a commit ac2886c
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ public String getClientPropertiesFile() {
File file = new File(path);
if (file.isFile() && file.canRead()) {
clientConfigFile = file.getAbsolutePath();
System.out.println("Loading configuration from " + clientConfigFile);
Shell.log.info("Loading configuration from {}", clientConfigFile);
break;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import static org.apache.accumulo.core.fate.FateTxId.parseTidFromUserInput;

import java.io.IOException;
import java.io.PrintWriter;
import java.lang.reflect.Type;
import java.util.ArrayList;
import java.util.Base64;
Expand Down Expand Up @@ -159,25 +160,25 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
if (cl.hasOption(cancel.getOpt())) {
String[] txids = cl.getOptionValues(cancel.getOpt());
validateArgs(txids);
System.out.println(
throw new ParseException(
"Option not available. Use 'accumulo admin fate -c " + String.join(" ", txids) + "'");
} else if (cl.hasOption(fail.getOpt())) {
String[] txids = cl.getOptionValues(fail.getOpt());
validateArgs(txids);
failedCommand = failTx(admin, zs, zk, managerLockPath, txids);
failedCommand = failTx(shellState.getWriter(), admin, zs, zk, managerLockPath, txids);
} else if (cl.hasOption(delete.getOpt())) {
String[] txids = cl.getOptionValues(delete.getOpt());
validateArgs(txids);
failedCommand = deleteTx(admin, zs, zk, managerLockPath, txids);
failedCommand = deleteTx(shellState.getWriter(), admin, zs, zk, managerLockPath, txids);
} else if (cl.hasOption(list.getOpt())) {
printTx(shellState, admin, zs, zk, tableLocksPath, cl.getOptionValues(list.getOpt()), cl);
} else if (cl.hasOption(print.getOpt())) {
printTx(shellState, admin, zs, zk, tableLocksPath, cl.getOptionValues(print.getOpt()), cl);
} else if (cl.hasOption(summary.getOpt())) {
System.out.println("Option not available. Use 'accumulo admin fate --summary'");
throw new ParseException("Option not available. Use 'accumulo admin fate --summary'");
} else if (cl.hasOption(dump.getOpt())) {
String output = dumpTx(zs, cl.getOptionValues(dump.getOpt()));
System.out.println(output);
shellState.getWriter().println(output);
} else {
throw new ParseException("Invalid command option");
}
Expand Down Expand Up @@ -234,14 +235,14 @@ protected void printTx(Shell shellState, AdminUtil<FateCommand> admin, ZooStore<
!cl.hasOption(disablePaginationOpt.getOpt()));
}

protected boolean deleteTx(AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs,
ZooReaderWriter zk, ServiceLockPath zLockManagerPath, String[] args)
protected boolean deleteTx(PrintWriter out, AdminUtil<FateCommand> admin,
ZooStore<FateCommand> zs, ZooReaderWriter zk, ServiceLockPath zLockManagerPath, String[] args)
throws InterruptedException, KeeperException {
for (int i = 1; i < args.length; i++) {
if (admin.prepDelete(zs, zk, zLockManagerPath, args[i])) {
admin.deleteLocks(zk, zLockManagerPath, args[i]);
} else {
System.out.printf("Could not delete transaction: %s%n", args[i]);
out.printf("Could not delete transaction: %s%n", args[i]);
return false;
}
}
Expand All @@ -254,12 +255,12 @@ private void validateArgs(String[] args) throws ParseException {
}
}

public boolean failTx(AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs, ZooReaderWriter zk,
ServiceLockPath managerLockPath, String[] args) {
public boolean failTx(PrintWriter out, AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs,
ZooReaderWriter zk, ServiceLockPath managerLockPath, String[] args) {
boolean success = true;
for (int i = 1; i < args.length; i++) {
if (!admin.prepFail(zs, zk, managerLockPath, args[i])) {
System.out.printf("Could not fail transaction: %s%n", args[i]);
out.printf("Could not fail transaction: %s%n", args[i]);
return !success;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.io.FileInputStream;
import java.io.IOException;
import java.io.PrintStream;
import java.io.PrintWriter;
import java.nio.file.Files;
import java.util.List;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -96,15 +97,15 @@ String dumpTx(ZooStore<FateCommand> zs, String[] args) {
}

@Override
protected boolean deleteTx(AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs,
ZooReaderWriter zk, ServiceLockPath zLockManagerPath, String[] args)
throws InterruptedException, KeeperException {
protected boolean deleteTx(PrintWriter out, AdminUtil<FateCommand> admin,
ZooStore<FateCommand> zs, ZooReaderWriter zk, ServiceLockPath zLockManagerPath,
String[] args) throws InterruptedException, KeeperException {
deleteCalled = true;
return true;
}

@Override
public boolean failTx(AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs,
public boolean failTx(PrintWriter out, AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs,
ZooReaderWriter zk, ServiceLockPath managerLockPath, String[] args) {
failCalled = true;
return true;
Expand Down Expand Up @@ -154,9 +155,10 @@ public void testFailTx() throws Exception {

FateCommand cmd = new FateCommand();
// require number for Tx
assertFalse(cmd.failTx(helper, zs, zk, managerLockPath, new String[] {"fail", "tx1"}));
var out = new PrintWriter(System.out);
assertFalse(cmd.failTx(out, helper, zs, zk, managerLockPath, new String[] {"fail", "tx1"}));
// fail the long configured above
assertTrue(cmd.failTx(helper, zs, zk, managerLockPath, new String[] {"fail", "12345"}));
assertTrue(cmd.failTx(out, helper, zs, zk, managerLockPath, new String[] {"fail", "12345"}));

verify(zs);
}
Expand Down

0 comments on commit ac2886c

Please sign in to comment.