dao-jun commented on code in PR #22209:
URL: https://github.com/apache/pulsar/pull/22209#discussion_r1524475741
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdGenerateDocument.java:
##########
@@ -99,21 +81,29 @@ private boolean needsLangSupport(String module, String
subK) {
return module.equals("functions") &&
Arrays.asList(langSupport).contains(subK);
}
- private void generateDocument(StringBuilder sb, String module,
JCommander obj) {
+ private final Set<String> generatedModule = new HashSet<>();
+
+ private void generateDocument(StringBuilder sb, String module,
CommandLine obj) {
+ // Filter the deprecated command
+ if (generatedModule.contains(module)) {
+ return;
+ }
+ String commandName = obj.getCommandName();
+ generatedModule.add(commandName);
Review Comment:
Why do we need to skip generated module? It seems changed the origin logics.
I don't thing it's necessary in this PR
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdPersistentTopics.java:
##########
@@ -93,420 +93,420 @@ private Topics getPersistentTopics() {
return persistentTopics;
}
- @Parameters(commandDescription = "Get the list of topics under a
namespace.")
+ @Command(description = "Get the list of topics under a namespace.")
private class ListCmd extends CliCommand {
- @Parameter(description = "tenant/namespace", required = true)
- private java.util.List<String> params;
+ @Parameters(description = "tenant/namespace", arity = "1")
+ private String namespaceName;
@Override
void run() throws PulsarAdminException {
- String namespace = validateNamespace(params);
+ String namespace = validateNamespace(namespaceName);
print(getPersistentTopics().getList(namespace));
}
}
- @Parameters(commandDescription = "Get the list of partitioned topics under
a namespace.")
+ @Command(description = "Get the list of partitioned topics under a
namespace.")
private class PartitionedTopicListCmd extends CliCommand {
- @Parameter(description = "tenant/namespace", required = true)
- private java.util.List<String> params;
+ @Parameters(description = "tenant/namespace", arity = "1")
+ private String namespaceName;
@Override
void run() throws PulsarAdminException {
- String namespace = validateNamespace(params);
+ String namespace = validateNamespace(namespaceName);
print(getPersistentTopics().getPartitionedTopicList(namespace));
}
}
- @Parameters(commandDescription = "Grant a new permission to a client role
on a single topic.")
+ @Command(description = "Grant a new permission to a client role on a
single topic.")
private class GrantPermissions extends CliCommand {
- @Parameter(description = "persistent://tenant/namespace/topic",
required = true)
- private java.util.List<String> params;
+ @Parameters(description = "persistent://tenant/namespace/topic", arity
= "1")
Review Comment:
Is `Parameter#arity` can fully replace CliCommand#getOneArgument ?
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdSinks.java:
##########
@@ -112,13 +110,7 @@ public CmdSinks(Supplier<PulsarAdmin> admin) {
abstract class BaseCommand extends CliCommand {
@Override
void run() throws Exception {
- try {
- processArguments();
- } catch (Exception e) {
- String chosenCommand = jcommander.getParsedCommand();
- getUsageFormatter().usage(chosenCommand);
- throw e;
- }
+ processArguments();
Review Comment:
Does here changed the origin logic? Or picocli handled the case by default?
##########
pulsar-client-tools/src/test/java/org/apache/pulsar/shell/AdminShellTest.java:
##########
@@ -48,25 +48,23 @@ public void test() throws Exception {
final PulsarAdmin admin = mock(PulsarAdmin.class);
when(builder.build()).thenReturn(admin);
when(admin.topics()).thenReturn(mock(Topics.class));
- adminShell.setPulsarAdminSupplier(new PulsarAdminSupplier(builder,
adminShell.getRootParams()));
+ PulsarAdminSupplier pulsarAdminSupplier =
adminShell.getPulsarAdminSupplier();
+ pulsarAdminSupplier.setAdminBuilder(builder);
assertTrue(run(new String[]{"topics", "list", "public/default"}));
- verify(builder).build();
+ verify(builder, times(1)).build();
assertTrue(run(new String[]{"topics", "list", "public/default"}));
- verify(builder).build();
+ verify(builder, times(1)).build();
assertTrue(run(new String[]{"--admin-url", "http://localhost:8081";,
"topics", "list", "public/default"}));
+ verify(builder, times(2)).build();
assertTrue(run(new String[]{"topics", "list", "public/default"}));
verify(builder, times(3)).build();
assertTrue(run(new String[]{"--admin-url", "http://localhost:8080";,
"topics", "list", "public/default"}));
verify(builder, times(3)).build();
}
- private boolean run(String[] args) throws Exception {
- try {
- return adminShell.runCommand(args);
- } finally {
- adminShell.cleanupState(props);
- }
+ private boolean run(String[] args) {
Review Comment:
Dont need to call `cleanupState` like jcommand?
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/PulsarAdminTool.java:
##########
@@ -41,75 +38,80 @@
import org.apache.pulsar.client.admin.PulsarAdminBuilder;
import org.apache.pulsar.client.admin.internal.PulsarAdminImpl;
import org.apache.pulsar.common.util.ShutdownUtil;
-
-public class PulsarAdminTool {
+import org.apache.pulsar.internal.CommandHook;
+import org.apache.pulsar.internal.CommanderFactory;
+import picocli.CommandLine;
+import picocli.CommandLine.ArgGroup;
+import picocli.CommandLine.Command;
+import picocli.CommandLine.Option;
+import picocli.CommandLine.ScopeType;
+
+@Command(name = "pulsar-admin",
+ scope = ScopeType.INHERIT,
+ mixinStandardHelpOptions = true,
+ showDefaultValues = true,
+ versionProvider = PulsarVersionProvider.class
+)
+public class PulsarAdminTool implements CommandHook {
protected static boolean allowSystemExit = true;
private static int lastExitCode = Integer.MIN_VALUE;
- protected final List<CustomCommandFactory> customCommandFactories;
+ protected List<CustomCommandFactory> customCommandFactories;
protected Map<String, Class<?>> commandMap;
- protected JCommander jcommander;
- protected RootParams rootParams;
- private final Properties properties;
+ protected final CommandLine commander;
+ @ArgGroup(heading = "Options:%n", exclusive = false)
+ protected RootParams rootParams = new RootParams();
protected PulsarAdminSupplier pulsarAdminSupplier;
+ private PulsarAdminPropertiesProvider pulsarAdminPropertiesProvider;
@Getter
public static class RootParams {
- @Parameter(names = { "--admin-url" }, description = "Admin Service URL
to which to connect.")
+ @Option(names = { "--admin-url" }, description = "Admin Service URL to
which to connect.",
+ descriptionKey = "webServiceUrl")
String serviceUrl = null;
- @Parameter(names = { "--auth-plugin" }, description = "Authentication
plugin class name.")
+ @Option(names = { "--auth-plugin" }, description = "Authentication
plugin class name.",
+ descriptionKey = "authPlugin")
String authPluginClassName = null;
- @Parameter(names = { "--request-timeout" }, description = "Request
time out in seconds for "
+ @Option(names = { "--request-timeout" }, description = "Request time
out in seconds for "
+ "the pulsar admin client for any request")
int requestTimeout = PulsarAdminImpl.DEFAULT_REQUEST_TIMEOUT_SECONDS;
- @Parameter(
- names = { "--auth-params" },
+ @Option(names = { "--auth-params" }, descriptionKey = "authParams",
description = "Authentication parameters, whose format is
determined by the implementation "
+ "of method `configure` in authentication plugin
class, for example \"key1:val1,key2:val2\" "
+ "or \"{\"key1\":\"val1\",\"key2\":\"val2\"}\".")
String authParams = null;
- @Parameter(names = { "--tls-allow-insecure" }, description = "Allow
TLS insecure connection")
+ @Option(names = { "--tls-allow-insecure" }, description = "Allow TLS
insecure connection")
Boolean tlsAllowInsecureConnection;
- @Parameter(names = { "--tls-trust-cert-path" }, description = "Allow
TLS trust cert file path")
+ @Option(names = { "--tls-trust-cert-path" }, description = "Allow TLS
trust cert file path")
String tlsTrustCertsFilePath;
- @Parameter(names = { "--tls-enable-hostname-verification" },
+ @Option(names = { "--tls-enable-hostname-verification" },
description = "Enable TLS common name verification")
Boolean tlsEnableHostnameVerification;
- @Parameter(names = {"--tls-provider"}, description = "Set up TLS
provider. "
+ @Option(names = {"--tls-provider"}, description = "Set up TLS
provider. "
+ "When TLS authentication with CACert is used, the valid
value is either OPENSSL or JDK. "
+ "When TLS authentication with KeyStore is used, available
options can be SunJSSE, Conscrypt "
- + "and so on.")
+ + "and so on.", descriptionKey = "webserviceTlsProvider")
String tlsProvider;
-
Review Comment:
also handled by picocli?
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/client/cli/CmdProduce.java:
##########
@@ -237,26 +238,30 @@ private static byte[] jsonToAvro(String m,
org.apache.avro.Schema avroSchema){
}
}
+ @Spec
+ private CommandSpec commandSpec;
+
/**
* Run the producer.
*
* @return 0 for success, < 0 otherwise
* @throws Exception
*/
public int run() throws PulsarClientException {
- if (mainOptions.size() != 1) {
- throw (new ParameterException("Please provide one and only one
topic name."));
- }
+ System.out.println("run");
Review Comment:
Do we need this line?
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/PulsarAdminTool.java:
##########
@@ -361,26 +295,23 @@ protected void initJCommander() {
// Automatically generate documents for pulsar-admin
commandMap.put("documents", CmdGenerateDocument.class);
// To remain backwards compatibility for "source" and "sink" commands
- // TODO eventually remove this
- commandMap.put("source", CmdSources.class);
Review Comment:
can we remove them directly?
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTenants.java:
##########
@@ -119,41 +114,34 @@ void run() throws PulsarAdminException {
}
}
- @Parameters(commandDescription = "Deletes an existing tenant")
+ @Command(description = "Deletes an existing tenant")
private class Delete extends CliCommand {
- @Parameter(description = "tenant-name", required = true)
- private java.util.List<String> params;
+ @Parameters(description = "tenant-name", arity = "1")
+ private String tenant;
- @Parameter(names = { "-f",
+ @Option(names = { "-f",
"--force" }, description = "Delete a tenant forcefully by
deleting all namespaces under it.")
private boolean force = false;
@Override
void run() throws PulsarAdminException {
- String tenant = getOneArgument(params);
getAdmin().tenants().deleteTenant(tenant, force);
}
}
public CmdTenants(Supplier<PulsarAdmin> admin) {
super("tenants", admin);
- jcommander.addCommand("list", new List());
- jcommander.addCommand("get", new Get());
- jcommander.addCommand("create", new Create());
- jcommander.addCommand("update", new Update());
- jcommander.addCommand("delete", new Delete());
+ addCommand("list", new List());
+ addCommand("get", new Get());
+ addCommand("create", new Create());
+ addCommand("update", new Update());
+ addCommand("delete", new Delete());
}
- @Parameters(hidden = true)
+ @Command(hidden = true)
static class CmdProperties extends CmdTenants {
public CmdProperties(Supplier<PulsarAdmin> admin) {
super(admin);
}
-
- @Override
- public boolean run(String[] args) {
- System.err.println("WARN: The properties subcommand is deprecated.
Please use tenants instead");
Review Comment:
picocli also handled the case?
--
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]
