You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2024/07/03 00:37:02 UTC
Re: [PR] supporting additional headers [pinot]
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org