zuston commented on code in PR #931:
URL: https://github.com/apache/incubator-uniffle/pull/931#discussion_r1222761992
##########
cli/src/main/java/org/apache/uniffle/cli/UniffleCLI.java:
##########
@@ -66,14 +91,40 @@ public int run(String[] args) throws
UniffleCliArgsException {
System.out.println("uniffle-admin-cli : " + cliArgs);
return 0;
}
+ if (cmd.hasOption(coordServer.getOpt()) &&
cmd.hasOption(coordPort.getOpt())) {
+ String host = cmd.getOptionValue(coordServer.getOpt()).trim();
+ int port =
Integer.parseInt(cmd.getOptionValue(coordPort.getOpt()).trim());
+ String hostUrl = String.format("http://%s:%d";, host, port);
+ LOG.info("uniffle-admin-cli : coordinator server host {}, port {}.",
host, port);
Review Comment:
If I understand correctly, the host:port is for all cli options rather than
only for admin. So this log is not accurate.
##########
cli/src/main/java/org/apache/uniffle/cli/UniffleCLI.java:
##########
@@ -25,28 +25,53 @@
import org.apache.uniffle.AbstractCustomCommandLine;
import org.apache.uniffle.UniffleCliArgsException;
+import org.apache.uniffle.api.AdminRestApi;
+import org.apache.uniffle.client.UniffleRestClient;
+
public class UniffleCLI extends AbstractCustomCommandLine {
private static final Logger LOG = LoggerFactory.getLogger(UniffleCLI.class);
private final Options allOptions;
private final Option uniffleClientCli;
private final Option uniffleAdminCli;
+ private final Option checkerClass;
+ private final Option coordServer;
+ private final Option coordPort;
+ private final Option refreshCheckerCli;
private final Option help;
+ protected UniffleRestClient client;
public UniffleCLI(String shortPrefix, String longPrefix) {
allOptions = new Options();
uniffleClientCli = new Option(shortPrefix + "c", longPrefix + "cli",
true, "This is an client cli command that will print args.");
uniffleAdminCli = new Option(shortPrefix + "a", longPrefix + "admin",
true, "This is an admin command that will print args.");
+ refreshCheckerCli = new Option(shortPrefix + "rc", longPrefix +
"refreshChecker",
+ false, "This is an admin command that will refresh access checker.");
+ checkerClass = new Option(shortPrefix + "cc", longPrefix + "checkerClass",
+ true, "This is the checker class that will be refreshed.");
+ coordServer = new Option(shortPrefix + "s", longPrefix + "host",
Review Comment:
coorServer -> coordinatorHost?
##########
cli/src/main/java/org/apache/uniffle/cli/UniffleCLI.java:
##########
@@ -25,28 +25,53 @@
import org.apache.uniffle.AbstractCustomCommandLine;
import org.apache.uniffle.UniffleCliArgsException;
+import org.apache.uniffle.api.AdminRestApi;
+import org.apache.uniffle.client.UniffleRestClient;
+
public class UniffleCLI extends AbstractCustomCommandLine {
private static final Logger LOG = LoggerFactory.getLogger(UniffleCLI.class);
private final Options allOptions;
private final Option uniffleClientCli;
private final Option uniffleAdminCli;
+ private final Option checkerClass;
+ private final Option coordServer;
+ private final Option coordPort;
+ private final Option refreshCheckerCli;
private final Option help;
+ protected UniffleRestClient client;
public UniffleCLI(String shortPrefix, String longPrefix) {
allOptions = new Options();
uniffleClientCli = new Option(shortPrefix + "c", longPrefix + "cli",
true, "This is an client cli command that will print args.");
uniffleAdminCli = new Option(shortPrefix + "a", longPrefix + "admin",
true, "This is an admin command that will print args.");
+ refreshCheckerCli = new Option(shortPrefix + "rc", longPrefix +
"refreshChecker",
+ false, "This is an admin command that will refresh access checker.");
+ checkerClass = new Option(shortPrefix + "cc", longPrefix + "checkerClass",
Review Comment:
The class name should be progagted to this options. It's weired and complex.
But I have no ideas on this
##########
cli/src/main/java/org/apache/uniffle/cli/UniffleCLI.java:
##########
@@ -66,14 +91,40 @@ public int run(String[] args) throws
UniffleCliArgsException {
System.out.println("uniffle-admin-cli : " + cliArgs);
return 0;
}
+ if (cmd.hasOption(coordServer.getOpt()) &&
cmd.hasOption(coordPort.getOpt())) {
+ String host = cmd.getOptionValue(coordServer.getOpt()).trim();
+ int port =
Integer.parseInt(cmd.getOptionValue(coordPort.getOpt()).trim());
+ String hostUrl = String.format("http://%s:%d";, host, port);
+ LOG.info("uniffle-admin-cli : coordinator server host {}, port {}.",
host, port);
Review Comment:
How about this? `connected to coordinator: http://xxxxxx:xxxxxxx`
##########
cli/src/main/java/org/apache/uniffle/cli/UniffleCLI.java:
##########
@@ -25,28 +25,53 @@
import org.apache.uniffle.AbstractCustomCommandLine;
import org.apache.uniffle.UniffleCliArgsException;
+import org.apache.uniffle.api.AdminRestApi;
+import org.apache.uniffle.client.UniffleRestClient;
+
public class UniffleCLI extends AbstractCustomCommandLine {
private static final Logger LOG = LoggerFactory.getLogger(UniffleCLI.class);
private final Options allOptions;
private final Option uniffleClientCli;
private final Option uniffleAdminCli;
+ private final Option checkerClass;
+ private final Option coordServer;
+ private final Option coordPort;
+ private final Option refreshCheckerCli;
private final Option help;
+ protected UniffleRestClient client;
public UniffleCLI(String shortPrefix, String longPrefix) {
allOptions = new Options();
uniffleClientCli = new Option(shortPrefix + "c", longPrefix + "cli",
true, "This is an client cli command that will print args.");
uniffleAdminCli = new Option(shortPrefix + "a", longPrefix + "admin",
true, "This is an admin command that will print args.");
+ refreshCheckerCli = new Option(shortPrefix + "rc", longPrefix +
"refreshChecker",
+ false, "This is an admin command that will refresh access checker.");
+ checkerClass = new Option(shortPrefix + "cc", longPrefix + "checkerClass",
+ true, "This is the checker class that will be refreshed.");
+ coordServer = new Option(shortPrefix + "s", longPrefix + "host",
Review Comment:
BTW, I prefer loading port and host from the default config. And users also
could overwrite this by above option. Maybe this could supported in another PR
##########
cli/src/main/java/org/apache/uniffle/cli/UniffleCLI.java:
##########
@@ -66,14 +91,40 @@ public int run(String[] args) throws
UniffleCliArgsException {
System.out.println("uniffle-admin-cli : " + cliArgs);
return 0;
}
+ if (cmd.hasOption(coordServer.getOpt()) &&
cmd.hasOption(coordPort.getOpt())) {
+ String host = cmd.getOptionValue(coordServer.getOpt()).trim();
+ int port =
Integer.parseInt(cmd.getOptionValue(coordPort.getOpt()).trim());
+ String hostUrl = String.format("http://%s:%d";, host, port);
+ LOG.info("uniffle-admin-cli : coordinator server host {}, port {}.",
host, port);
+ client = UniffleRestClient.builder(hostUrl).build();
+ }
+
+ if (cmd.hasOption(refreshCheckerCli.getOpt())) {
+ String checker = cmd.getOptionValue(checkerClass.getOpt());
Review Comment:
If the refreshChecker cli exists, the checkerClass must exist, right? If so,
we should fast fail when this condition is not satisfied.
Also, here the checker should be checked whether is null.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
