walterddr commented on a change in pull request #7665:
URL: https://github.com/apache/pinot/pull/7665#discussion_r740374934
##########
File path: pom.xml
##########
@@ -1281,6 +1281,11 @@
<artifactId>args4j</artifactId>
Review comment:
not yet. we still have places other than PinotAdministrator that still
uses args4j. there are also some project that extends pinot-tools which will be
using args4j transitively.
Will completely remove args4j in the next couple of PRs once I migrate
everything
##########
File path:
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -53,34 +52,35 @@
* <li>All remaining bootstrap services in parallel</li>
* </ol>
*/
[email protected](name = "StartServiceManager")
public class StartServiceManagerCommand extends AbstractBaseAdminCommand
implements Command {
private static final Logger LOGGER =
LoggerFactory.getLogger(StartServiceManagerCommand.class);
private static final long START_TICK = System.nanoTime();
private static final String[] BOOTSTRAP_SERVICES = new
String[]{"CONTROLLER", "BROKER", "SERVER"};
// multiple instances allowed per role for testing many minions
private final List<Entry<ServiceRole, Map<String, Object>>>
_bootstrapConfigurations = new ArrayList<>();
- @Option(name = "-help", required = false, help = true, aliases = {"-h",
"--h", "--help"},
- usage = "Print this message.")
+ @CommandLine.Option(names = {"-help", "-h", "--h", "--help"}, required =
false, help = true,
+ description = "Print this message.")
private boolean _help;
- @Option(name = "-zkAddress", required = false, metaVar = "<http>", usage =
"Http address of Zookeeper.",
- forbids = {"-bootstrapConfigPaths", "-bootstrapServices"})
+ @CommandLine.Option(names = {"-zkAddress"}, required = false, description =
"Http address of Zookeeper.")
+ // TODO: support forbids = {"-bootstrapConfigPaths", "-bootstrapServices"})
private String _zkAddress = DEFAULT_ZK_ADDRESS;
- @Option(name = "-clusterName", required = false, metaVar = "<String>", usage
= "Pinot cluster name.",
- forbids = {"-bootstrapConfigPaths", "-bootstrapServices"})
+ @CommandLine.Option(names = {"-clusterName"}, required = false, description
= "Pinot cluster name.")
+ // TODO: support forbids = {"-bootstrapConfigPaths",
"-bootstrapServices"})
private String _clusterName = DEFAULT_CLUSTER_NAME;
- @Option(name = "-port", required = false, metaVar = "<int>",
- usage = "Pinot service manager admin port, -1 means disable, 0 means a
random available port.",
- forbids = {"-bootstrapConfigPaths", "-bootstrapServices"})
+ @CommandLine.Option(names = {"-port"}, required = false,
+ description = "Pinot service manager admin port, -1 means disable, 0
means a random available port.")
+ // TODO: support forbids = {"-bootstrapConfigPaths",
"-bootstrapServices"})
private int _port = -1;
- @Option(name = "-bootstrapConfigPaths", handler =
StringArrayOptionHandler.class, required = false,
- usage = "A list of Pinot service config file paths. Each config file
requires an extra config: 'pinot.service"
- + ".role' to indicate which service to start.",
- forbids = {"-zkAddress", "-clusterName", "-port", "-bootstrapServices"})
+ @CommandLine.Option(names = {"-bootstrapConfigPaths"}, required = false,
arity = "1..*",
+ description = "A list of Pinot service config file paths. Each config
file requires an extra config:"
+ + " 'pinot.service.role' to indicate which service to start.")
+ // TODO: support forbids = {"-zkAddress", "-clusterName", "-port",
"-bootstrapServices"})
private String[] _bootstrapConfigPaths;
- @Option(name = "-bootstrapServices", handler =
StringArrayOptionHandler.class, required = false,
- usage = "A list of Pinot service roles to start with default config.
E.g. CONTROLLER/BROKER/SERVER",
- forbids = {"-zkAddress", "-clusterName", "-port",
"-bootstrapConfigPaths"})
+ @CommandLine.Option(names = {"-bootstrapServices"}, required = false, arity
= "1..*",
Review comment:
yes. this is tested in Quickstart CI jobs
##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/Command.java
##########
@@ -18,12 +18,21 @@
*/
package org.apache.pinot.tools;
+import java.util.concurrent.Callable;
+
+
/**
* Interface class for pinot-admin commands.
*
*
*/
-public interface Command {
+public interface Command extends Callable<Integer> {
+
+ default Integer call() throws Exception {
+ // run execute() and returns 0 if success otherwise return -1.
+ return execute() ? 0 : -1;
Review comment:
i think any non-zero return code indicates a failure. but yes I can keep
it consistent if previously it is using `1` as failure return code.
##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/Command.java
##########
@@ -18,12 +18,21 @@
*/
package org.apache.pinot.tools;
+import java.util.concurrent.Callable;
+
+
/**
* Interface class for pinot-admin commands.
*
*
*/
-public interface Command {
+public interface Command extends Callable<Integer> {
+
+ default Integer call() throws Exception {
+ // run execute() and returns 0 if success otherwise return -1.
+ return execute() ? 0 : -1;
Review comment:
```suggestion
return execute() ? 0 : 1;
```
--
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]
