Jackie-Jiang commented on code in PR #9471:
URL: https://github.com/apache/pinot/pull/9471#discussion_r981856425
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransport.java:
##########
@@ -66,7 +66,8 @@ public JsonAsyncHttpPinotClientTransport() {
}
public JsonAsyncHttpPinotClientTransport(Map<String, String> headers, String
scheme,
- @Nullable SSLContext sslContext, ConnectionTimeouts connectionTimeouts,
TlsProtocols tlsProtocols) {
+ @Nullable SSLContext sslContext,
ConnectionTimeouts connectionTimeouts,
Review Comment:
(format) Please follow the [Pinot
Style](https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#setup-ide).
Same for other changes
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/utils/ConnectionUtils.java:
##########
@@ -56,14 +56,18 @@ public static SSLContext
getSSLContextFromProperties(Properties properties) {
}
- public static String getUserAgentVersionFromClassPath(String userAgentKey) {
+ public static String getUserAgentVersionFromClassPath(String userAgentKey,
String appId) {
Review Comment:
```suggestion
public static String getUserAgentVersionFromClassPath(String userAgentKey,
@Nullable String appId) {
```
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/utils/ConnectionUtils.java:
##########
@@ -56,14 +56,18 @@ public static SSLContext
getSSLContextFromProperties(Properties properties) {
}
- public static String getUserAgentVersionFromClassPath(String userAgentKey) {
+ public static String getUserAgentVersionFromClassPath(String userAgentKey,
String appId) {
Properties userAgentProperties = new Properties();
try {
userAgentProperties.load(ConnectionUtils.class.getClassLoader()
.getResourceAsStream("version.properties"));
} catch (IOException e) {
LOGGER.warn("Unable to set user agent version");
}
- return userAgentProperties.getProperty(userAgentKey, "pinot-java");
+ String userAgentFromProperties =
userAgentProperties.getProperty(userAgentKey, "unknown");
+ if (appId != null && appId.length() > 0) {
Review Comment:
(nit) StringUtils.isNotEmpty(appId)
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransportFactory.java:
##########
@@ -94,6 +96,7 @@ public JsonAsyncHttpPinotClientTransportFactory
withConnectionProperties(Propert
DEFAULT_BROKER_CONNECT_TIMEOUT_MS));
_handshakeTimeoutMs =
Integer.parseInt(properties.getProperty("brokerHandshakeTimeoutMs",
DEFAULT_BROKER_HANDSHAKE_TIMEOUT_MS));
+ _appId = properties.getProperty("appId", "");
Review Comment:
Set it to `null` if not available
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransport.java:
##########
@@ -66,7 +66,8 @@ public JsonAsyncHttpPinotClientTransport() {
}
public JsonAsyncHttpPinotClientTransport(Map<String, String> headers, String
scheme,
- @Nullable SSLContext sslContext, ConnectionTimeouts connectionTimeouts,
TlsProtocols tlsProtocols) {
+ @Nullable SSLContext sslContext,
ConnectionTimeouts connectionTimeouts,
Review Comment:
Annotate `appId` as `@Nullable`
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransportFactory.java:
##########
@@ -43,13 +43,15 @@ public class JsonAsyncHttpPinotClientTransportFactory
implements PinotClientTran
private int _readTimeoutMs =
Integer.parseInt(DEFAULT_BROKER_READ_TIMEOUT_MS);
private int _connectTimeoutMs =
Integer.parseInt(DEFAULT_BROKER_READ_TIMEOUT_MS);
private int _handshakeTimeoutMs =
Integer.parseInt(DEFAULT_BROKER_HANDSHAKE_TIMEOUT_MS);
+ private String _appId = "";
Review Comment:
Suggest setting it to `null` if not available
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransport.java:
##########
@@ -52,7 +52,8 @@ public class PinotControllerTransport {
public PinotControllerTransport(Map<String, String> headers, String scheme,
- @Nullable SSLContext sslContext, ConnectionTimeouts connectionTimeouts,
TlsProtocols tlsProtocols) {
+ @Nullable SSLContext sslContext,
ConnectionTimeouts connectionTimeouts,
Review Comment:
Same comments as in `JsonAsyncHttpPinotClientTransport`
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransportFactory.java:
##########
@@ -41,12 +41,13 @@ public class PinotControllerTransportFactory {
private int _readTimeoutMs =
Integer.parseInt(DEFAULT_CONTROLLER_READ_TIMEOUT_MS);
private int _connectTimeoutMs =
Integer.parseInt(DEFAULT_CONTROLLER_CONNECT_TIMEOUT_MS);
private int _handshakeTimeoutMs =
Integer.parseInt(DEFAULT_CONTROLLER_HANDSHAKE_TIMEOUT_MS);
+ private String _appId = "";
Review Comment:
Same comments as in `JsonAsyncHttpPinotClientTransportFactory`
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
