Jackie-Jiang commented on code in PR #13417:
URL: https://github.com/apache/pinot/pull/13417#discussion_r1663326337
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/DeleteSchemaCommand.java:
##########
@@ -150,8 +90,9 @@ public boolean execute()
try (FileUploadDownloadClient fileUploadDownloadClient = new
FileUploadDownloadClient()) {
fileUploadDownloadClient.getHttpClient().sendDeleteRequest(
FileUploadDownloadClient.getDeleteSchemaURI(_controllerProtocol,
_controllerHost,
- Integer.parseInt(_controllerPort), _schemaName),
Collections.emptyMap(),
- AuthProviderUtils.makeAuthProvider(_authProvider, _authTokenUrl,
_authToken, _user, _password));
+ Integer.parseInt(_controllerPort), _schemaName),
getHeadersAsMap(),
Review Comment:
(minor, format) Reformat
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/DeleteTableCommand.java:
##########
@@ -172,8 +111,9 @@ public boolean execute()
try (FileUploadDownloadClient fileUploadDownloadClient = new
FileUploadDownloadClient()) {
fileUploadDownloadClient.getHttpClient().sendDeleteRequest(FileUploadDownloadClient
.getDeleteTableURI(_controllerProtocol, _controllerHost,
Integer.parseInt(_controllerPort),
- _tableName, _type, _retention), Collections.emptyMap(),
AuthProviderUtils.makeAuthProvider(_authProvider,
- _authTokenUrl, _authToken, _user, _password));
+ _tableName, _type, _retention), getHeadersAsMap(),
Review Comment:
(minor, format) Reformat this
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/AddSchemaCommand.java:
##########
@@ -58,23 +46,8 @@ public class AddSchemaCommand extends
AbstractBaseAdminCommand implements Comman
+ "the schema exists.")
private boolean _force = false;
- @CommandLine.Option(names = {"-exec"}, required = false, description =
"Execute the command.")
- private boolean _exec;
-
- @CommandLine.Option(names = {"-user"}, required = false, description =
"Username for basic auth.")
- private String _user;
-
- @CommandLine.Option(names = {"-password"}, required = false, description =
"Password for basic auth.")
- private String _password;
-
- @CommandLine.Option(names = {"-authToken"}, required = false, description =
"Http auth token.")
- private String _authToken;
-
- @CommandLine.Option(names = {"-authTokenUrl"}, required = false, description
= "Http auth token url.")
- private String _authTokenUrl;
-
- @CommandLine.Option(names = {"-help", "-h", "--h", "--help"}, required =
false, help = true,
- description = "Print this message.")
+ @CommandLine.Option(names = {"-help", "-h", "--h", "--help"}, required =
false, help = true, description = "Print "
+ + "this message.")
private boolean _help = false;
private AuthProvider _authProvider;
Review Comment:
Remove this
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/DeleteTableCommand.java:
##########
@@ -45,30 +42,6 @@ public class DeleteTableCommand extends
AbstractBaseAdminCommand implements Comm
+ "the cluster setting, then '7d'. Using 0d or -1d will instantly delete
segments without retention.")
private String _retention;
- @CommandLine.Option(names = {"-controllerHost"}, required = false,
description = "Host name for controller.")
- private String _controllerHost;
-
- @CommandLine.Option(names = {"-controllerPort"}, required = false,
description = "Port number for controller.")
- private String _controllerPort = DEFAULT_CONTROLLER_PORT;
-
- @CommandLine.Option(names = {"-controllerProtocol"}, required = false,
description = "Protocol for controller.")
- private String _controllerProtocol = CommonConstants.HTTP_PROTOCOL;
-
- @CommandLine.Option(names = {"-exec"}, required = false, description =
"Execute the command.")
- private boolean _exec;
-
- @CommandLine.Option(names = {"-user"}, required = false, description =
"Username for basic auth.")
- private String _user;
-
- @CommandLine.Option(names = {"-password"}, required = false, description =
"Password for basic auth.")
- private String _password;
-
- @CommandLine.Option(names = {"-authToken"}, required = false, description =
"Http auth token.")
- private String _authToken;
-
- @CommandLine.Option(names = {"-authTokenUrl"}, required = false, description
= "Http auth token url.")
- private String _authTokenUrl;
-
@CommandLine.Option(names = {"-help", "-h", "--h", "--help"}, required =
false, help = true, description = "Print "
+ "this message.")
private boolean _help = false;
Review Comment:
Is it possible to also move `_help` into the base class? It is repeated in
all commands
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/AddSchemaCommand.java:
##########
@@ -97,8 +70,8 @@ public String getName() {
@Override
public String toString() {
String retString = ("AddSchema -controllerProtocol " + _controllerProtocol
+ " -controllerHost " + _controllerHost
- + " -controllerPort " + _controllerPort + " -schemaFile " +
_schemaFile + " -override " + _override + " _force "
- + _force + " -user " + _user + " -password " + "[hidden]");
+ + " -controllerPort " + _controllerPort + " -database " + _database +
" -schemaFile " + _schemaFile
+ + " -override " + _override + " _force " + _force + " -user " + _user
+ " -password " + "[hidden]");
Review Comment:
Consider adding a method in the base class to print the info managed in the
base class, then only append extra fields here. Currently it doesn't cover all
the supported fields
--
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]
