Copilot commented on code in PR #17167:
URL: https://github.com/apache/pinot/pull/17167#discussion_r2505959570
##########
pinot-connectors/pinot-flink-connector/src/main/java/org/apache/pinot/connector/flink/FlinkQuickStart.java:
##########
@@ -79,10 +77,8 @@ public static void main(String[] args)
execEnv.setParallelism(2);
DataStream<Row> srcDs =
execEnv.fromCollection(data).returns(TEST_TYPE_INFO).keyBy(r -> r.getField(0));
- HttpClient httpClient = HttpClient.getInstance();
- ControllerRequestClient client = new ControllerRequestClient(
- ControllerRequestURLBuilder.baseUrl(DEFAULT_CONTROLLER_URL),
httpClient);
- Schema schema = PinotConnectionUtils.getSchema(client, "starbucksStores");
+ PinotAdminClient client = new PinotAdminClient("localhost:9000");
+ Schema schema = client.getSchemaClient().getSchema("starbucksStores");
Review Comment:
The `getSchema()` method now returns a Schema object instead of a String,
but this line treats it as if it returns a String. This will cause a
compilation error since the Schema type from `getSchemaClient().getSchema()` is
being used directly without conversion.
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/admin/PinotSchemaAdminClient.java:
##########
@@ -69,17 +71,30 @@ public String getSchemasInfo()
}
/**
- * Gets a specific schema by name.
+ * Gets a specific schema string by name.
*
* @param schemaName Name of the schema
* @return Schema configuration as JSON string
* @throws PinotAdminException If the request fails
*/
- public String getSchema(String schemaName)
+ public String getSchemaString(String schemaName)
Review Comment:
The method `getSchemaString()` is introduced but lacks a corresponding
update to the overloaded `getSchema()` method's documentation. Since both
methods exist with different return types, their relationship and use cases
should be clarified in the class-level documentation or method comments.
##########
pinot-connectors/pinot-flink-connector/src/main/java/org/apache/pinot/connector/flink/http/PinotConnectionUtils.java:
##########
@@ -58,7 +64,7 @@ public static TableConfig
getTableConfig(ControllerRequestClient client, String
Map<String, String> newBatchConfigMap = new HashMap<>();
// append the batch config of controller URI
- String controllerBaseUrl =
client.getControllerRequestURLBuilder().getBaseUrl();
+ String controllerBaseUrl = "http://"; + client.getControllerAddress();
Review Comment:
Hard-coding the scheme as 'http://' ignores the transport's actual scheme
configuration. This will fail when HTTPS is required. Use
`client.getControllerBaseUrl()` instead, which properly incorporates the scheme
from the transport.
```suggestion
String controllerBaseUrl = client.getControllerBaseUrl();
```
--
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]
