Copilot commented on code in PR #17167:
URL: https://github.com/apache/pinot/pull/17167#discussion_r3042406006
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/ChangeTableState.java:
##########
@@ -36,71 +30,49 @@
@CommandLine.Command(name = "ChangeTableState", mixinStandardHelpOptions =
true)
-public class ChangeTableState extends AbstractBaseAdminCommand implements
Command {
+public class ChangeTableState extends AbstractDatabaseBaseAdminCommand
implements Command {
private static final Logger LOGGER =
LoggerFactory.getLogger(ChangeTableState.class);
- @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 = {"-tableName"}, required = true, description =
"Table name to disable")
private String _tableName;
+ @CommandLine.Option(names = {"-tableType"}, required = false,
+ description = "Table type (OFFLINE|REALTIME). Optional if tableName
already has a type suffix.")
+ private TableType _tableType;
+
@CommandLine.Option(names = {"-state"}, required = true, description =
"Change Table State(enable|disable|drop)")
private String _state;
- @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;
-
- private AuthProvider _authProvider;
-
- public ChangeTableState setAuthProvider(AuthProvider authProvider) {
- _authProvider = authProvider;
- return this;
- }
-
@Override
public boolean execute()
throws Exception {
if (_controllerHost == null) {
_controllerHost = NetUtils.getHostAddress();
}
- String stateValue = _state.toLowerCase();
+ String stateValue = _state.toLowerCase(Locale.ROOT);
if (!stateValue.equals("enable") && !stateValue.equals("disable") &&
!stateValue.equals("drop")) {
throw new IllegalArgumentException(
"Invalid value for state: " + _state + "\n Value must be one of
enable|disable|drop");
}
- try (CloseableHttpClient httpClient = HttpClientBuilder.create().build()) {
- URI uri = new URI(_controllerProtocol, null, _controllerHost,
Integer.parseInt(_controllerPort),
- URI_TABLES_PATH + _tableName, "state=" + stateValue, null);
-
- HttpGet httpGet = new HttpGet(uri);
- AuthProviderUtils.makeAuthHeaders(
- AuthProviderUtils.makeAuthProvider(_authProvider, _authTokenUrl,
_authToken, _user, _password))
- .forEach(header -> httpGet.addHeader(header.getName(),
header.getValue()));
-
- try (CloseableHttpResponse response = httpClient.execute(httpGet)) {
- int status = response.getCode();
- if (status != 200) {
- String responseString = EntityUtils.toString(response.getEntity());
- throw new HttpException("Failed to change table state, error: " +
responseString);
- }
+ String tableType = _tableType != null ? _tableType.name() : null;
+ try (PinotAdminClient adminClient = getPinotAdminClient()) {
+ switch (stateValue) {
+ case "enable":
+ adminClient.getTableClient().setTableState(_tableName, tableType,
true);
+ break;
+ case "disable":
+ adminClient.getTableClient().setTableState(_tableName, tableType,
false);
+ break;
+ case "drop":
+ adminClient.getTableClient().deleteTable(_tableName, tableType,
null, null);
+ break;
Review Comment:
`-tableType` is documented as optional when `-tableName` already includes a
type suffix, but the controller `/tables/{tableName}/state` API requires the
`type` query param. When `_tableType` is null, this sends no `type` and will
fail with 400. Also, if callers pass a typed table name (e.g. `foo_OFFLINE`)
and a `type`, the controller will append the suffix again (e.g.
`foo_OFFLINE_OFFLINE`). Consider: (1) parsing `_tableName` to extract raw name
+ inferred type when `_tableType` is null, and (2) always passing the raw name
in the path and the inferred/explicit type as the query param; or make
`-tableType` required and remove the “optional” claim.
##########
pinot-connectors/pinot-flink-connector/README.md:
##########
@@ -57,9 +57,9 @@ ControllerRequestClient client = new ControllerRequestClient(
ControllerRequestURLBuilder.baseUrl(DEFAULT_CONTROLLER_URL), httpClient);
// fetch Pinot schema
-Schema schema = PinotConnectionUtils.getSchema(client, "starbucksStores");
+Schema schema = client.getSchemaClient().getSchemaObject("starbucksStores");
// fetch Pinot table config
-TableConfig tableConfig = PinotConnectionUtils.getTableConfig(client,
"starbucksStores", "REALTIME");
+TableConfig tableConfig =
client.getTableClient().getTableConfigObject("starbucksStores", "REALTIME");
Review Comment:
Same issue as the offline quick-start snippet above: this example still uses
`ControllerRequestClient` but calls `getSchemaClient()`/`getTableClient()`
methods that exist on `PinotAdminClient`. Please switch the snippet to use
`PinotAdminClient` so it compiles and matches the migrated code.
##########
pinot-connectors/pinot-flink-connector/README.md:
##########
@@ -36,9 +36,9 @@ ControllerRequestClient client = new ControllerRequestClient(
ControllerRequestURLBuilder.baseUrl(DEFAULT_CONTROLLER_URL), httpClient);
// fetch Pinot schema
-Schema schema = PinotConnectionUtils.getSchema(client, "starbucksStores");
+Schema schema = client.getSchemaClient().getSchemaObject("starbucksStores");
// fetch Pinot table config
-TableConfig tableConfig = PinotConnectionUtils.getTableConfig(client,
"starbucksStores", "OFFLINE");
+TableConfig tableConfig =
client.getTableClient().getTableConfigObject("starbucksStores", "OFFLINE");
// create Flink Pinot Sink
Review Comment:
The README snippet still constructs a
`ControllerRequestClient`/`HttpClient`, but then calls
`getSchemaClient()`/`getTableClient()` which are `PinotAdminClient` APIs. As
written, this example won’t compile and doesn’t reflect the new recommended
client. Update the snippet to instantiate `PinotAdminClient` (preferably with
try-with-resources) and remove the `ControllerRequestClient` setup.
--
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]
