You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/09/28 01:31:00 UTC
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9471: make first part of user agent header configurable
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-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