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/27 09:44:30 UTC
[GitHub] [pinot] rino-kadijk opened a new pull request, #9471: make first part of user agent header configurable
rino-kadijk opened a new pull request, #9471:
URL: https://github.com/apache/pinot/pull/9471
This PR adds the configuration option to prefix the User Agent header with a custom appId. This allows the server to see where the queries originate from.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/User-Agent#library_and_net_tool_ua_strings
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #9471: make first part of user agent header configurable
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9471:
URL: https://github.com/apache/pinot/pull/9471#issuecomment-1260302720
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9471?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#9471](https://codecov.io/gh/apache/pinot/pull/9471?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4b60900) into [master](https://codecov.io/gh/apache/pinot/commit/83b7f157f77c07675d7760569a796199a41b5555?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (83b7f15) will **decrease** coverage by `1.27%`.
> The diff coverage is `68.18%`.
```diff
@@ Coverage Diff @@
## master #9471 +/- ##
============================================
- Coverage 69.89% 68.61% -1.28%
- Complexity 4742 4809 +67
============================================
Files 1910 1910
Lines 101787 101798 +11
Branches 15445 15447 +2
============================================
- Hits 71139 69844 -1295
- Misses 25628 26941 +1313
+ Partials 5020 5013 -7
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `26.09% <63.63%> (+0.05%)` | :arrow_up: |
| integration2 | `?` | |
| unittests1 | `67.14% <ø> (-0.01%)` | :arrow_down: |
| unittests2 | `15.49% <50.00%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...main/java/org/apache/pinot/client/BrokerCache.java](https://codecov.io/gh/apache/pinot/pull/9471/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Jyb2tlckNhY2hlLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/pinot/pull/9471/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `43.37% <50.00%> (-2.41%)` | :arrow_down: |
| [...org/apache/pinot/client/utils/ConnectionUtils.java](https://codecov.io/gh/apache/pinot/pull/9471/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L3V0aWxzL0Nvbm5lY3Rpb25VdGlscy5qYXZh) | `50.00% <50.00%> (-3.34%)` | :arrow_down: |
| [...ot/client/controller/PinotControllerTransport.java](https://codecov.io/gh/apache/pinot/pull/9471/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qZGJjLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L2NvbnRyb2xsZXIvUGlub3RDb250cm9sbGVyVHJhbnNwb3J0LmphdmE=) | `42.62% <83.33%> (-5.66%)` | :arrow_down: |
| [...ient/JsonAsyncHttpPinotClientTransportFactory.java](https://codecov.io/gh/apache/pinot/pull/9471/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydEZhY3RvcnkuamF2YQ==) | `58.82% <100.00%> (-9.93%)` | :arrow_down: |
| [...nt/controller/PinotControllerTransportFactory.java](https://codecov.io/gh/apache/pinot/pull/9471/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qZGJjLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L2NvbnRyb2xsZXIvUGlub3RDb250cm9sbGVyVHJhbnNwb3J0RmFjdG9yeS5qYXZh) | `60.71% <100.00%> (-20.06%)` | :arrow_down: |
| [...t/core/plan/StreamingInstanceResponsePlanNode.java](https://codecov.io/gh/apache/pinot/pull/9471/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1N0cmVhbWluZ0luc3RhbmNlUmVzcG9uc2VQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ore/operator/streaming/StreamingResponseUtils.java](https://codecov.io/gh/apache/pinot/pull/9471/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nUmVzcG9uc2VVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...server/starter/helix/SegmentReloadStatusValue.java](https://codecov.io/gh/apache/pinot/pull/9471/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9TZWdtZW50UmVsb2FkU3RhdHVzVmFsdWUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../operator/blocks/results/MetadataResultsBlock.java](https://codecov.io/gh/apache/pinot/pull/9471/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9ibG9ja3MvcmVzdWx0cy9NZXRhZGF0YVJlc3VsdHNCbG9jay5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [145 more](https://codecov.io/gh/apache/pinot/pull/9471/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [pinot] rino-kadijk commented on pull request #9471: make first part of user agent header configurable
Posted by GitBox <gi...@apache.org>.
rino-kadijk commented on PR #9471:
URL: https://github.com/apache/pinot/pull/9471#issuecomment-1282136113
> Please take a look at the failed tests
I don't see anything related to this failure in this PR:
```java.lang.NullPointerException
[20596](https://github.com/apache/pinot/actions/runs/3266516098/jobs/5370354908#step:5:20597)
at org.apache.pinot.segment.local.io.readerwriter.RealtimeIndexOffHeapMemoryManager.close(RealtimeIndexOffHeapMemoryManager.java:109)
[20597](https://github.com/apache/pinot/actions/runs/3266516098/jobs/5370354908#step:5:20598)
at org.apache.pinot.segment.local.indexsegment.mutable.MutableSegmentImpl.destroy(MutableSegmentImpl.java:1187)
[20598](https://github.com/apache/pinot/actions/runs/3266516098/jobs/5370354908#step:5:20599)
at org.apache.pinot.core.data.manager.realtime.LLRealtimeSegmentDataManager.destroy(LLRealtimeSegmentDataManager.java:1213)
[20599](https://github.com/apache/pinot/actions/runs/3266516098/jobs/5370354908#step:5:20600)
at org.apache.pinot.core.data.manager.realtime.RealtimeTableDataManager.doShutdown(RealtimeTableDataManager.java:210)
[20600](https://github.com/apache/pinot/actions/runs/3266516098/jobs/5370354908#step:5:20601)
at org.apache.pinot.core.data.manager.BaseTableDataManager.shutDown(BaseTableDataManager.java:190)
[20601](https://github.com/apache/pinot/actions/runs/3266516098/jobs/5370354908#step:5:20602)
at org.apache.pinot.server.starter.helix.HelixInstanceDataManager.shutDown(HelixInstanceDataManager.java:155)
[20602](https://github.com/apache/pinot/actions/runs/3266516098/jobs/5370354908#step:5:20603)
at org.apache.pinot.server.starter.ServerInstance.shutDown(ServerInstance.java:261)
[20603](https://github.com/apache/pinot/actions/runs/3266516098/jobs/5370354908#step:5:20604)
at org.apache.pinot.server.starter.helix.BaseServerStarter.stop(BaseServerStarter.java:624)
[20604](https://github.com/apache/pinot/actions/runs/3266516098/jobs/5370354908#step:5:20605)
at org.apache.pinot.integration.tests.ClusterTest.stopServer(ClusterTest.java:276)
[20605](https://github.com/apache/pinot/actions/runs/3266516098/jobs/5370354908#step:5:20606)
at org.apache.pinot.integration.tests.UrlAuthRealtimeIntegrationTest.tearDown(UrlAuthRealtimeIntegrationTest.java:91)```
--
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
[GitHub] [pinot] Jackie-Jiang commented on pull request #9471: make first part of user agent header configurable
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #9471:
URL: https://github.com/apache/pinot/pull/9471#issuecomment-1262805963
Is there a constraint on the appId length? If so, we should fail it when user configure it with a too long value
--
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
[GitHub] [pinot] rino-kadijk commented on a diff in pull request #9471: make first part of user agent header configurable
Posted by GitBox <gi...@apache.org>.
rino-kadijk commented on code in PR #9471:
URL: https://github.com/apache/pinot/pull/9471#discussion_r997218327
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransport.java:
##########
@@ -64,21 +66,26 @@ public PinotControllerTransport(Map<String, String> headers, String scheme,
builder.setReadTimeout(connectionTimeouts.getReadTimeoutMs())
.setConnectTimeout(connectionTimeouts.getConnectTimeoutMs())
.setHandshakeTimeout(connectionTimeouts.getHandshakeTimeoutMs())
- .setUserAgent(getUserAgentVersionFromClassPath())
+ .setUserAgent(getUserAgentVersionFromClassPath(appId))
.setEnabledProtocols(tlsProtocols.getEnabledProtocols().toArray(new String[0]));
_httpClient = Dsl.asyncHttpClient(builder.build());
}
- private String getUserAgentVersionFromClassPath() {
+ private String getUserAgentVersionFromClassPath(@Nullable String appId) {
Properties userAgentProperties = new Properties();
try {
userAgentProperties.load(PinotControllerTransport.class.getClassLoader()
.getResourceAsStream("version.properties"));
} catch (IOException e) {
LOGGER.warn("Unable to set user agent version");
}
- return userAgentProperties.getProperty("ua", "unknown");
+ String userAgentFromProperties = userAgentProperties.getProperty("ua", "unknown");
+ if (StringUtils.isNotEmpty(appId)) {
+ return appId.substring(0, Math.min(org.apache.pinot.client.utils.ConnectionUtils.APP_ID_MAX_CHARS,
Review Comment:
[WARNING] src/main/java/org/apache/pinot/client/controller/PinotControllerTransport.java:[45,60] (imports) AvoidStaticImport: Using a static member import should be avoided - org.apache.pinot.client.utils.ConnectionUtils.APP_ID_MAX_CHARS.
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9471: make first part of user agent header configurable
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9471: make first part of user agent header configurable
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9471:
URL: https://github.com/apache/pinot/pull/9471#discussion_r997513197
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransport.java:
##########
@@ -70,7 +70,8 @@ public JsonAsyncHttpPinotClientTransport() {
}
public JsonAsyncHttpPinotClientTransport(Map<String, String> headers, String scheme, String extraOptionString,
- @Nullable SSLContext sslContext, ConnectionTimeouts connectionTimeouts, TlsProtocols tlsProtocols) {
+ @Nullable SSLContext sslContext, ConnectionTimeouts connectionTimeouts,
Review Comment:
Please reformat this file
--
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
[GitHub] [pinot] rino-kadijk commented on pull request #9471: make first part of user agent header configurable
Posted by GitBox <gi...@apache.org>.
rino-kadijk commented on PR #9471:
URL: https://github.com/apache/pinot/pull/9471#issuecomment-1260971399
> LGTM in general. Minor comments
will fix, should I trim the appId to 32 characters?
--
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
[GitHub] [pinot] Jackie-Jiang merged pull request #9471: make first part of user agent header configurable
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged PR #9471:
URL: https://github.com/apache/pinot/pull/9471
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9471: make first part of user agent header configurable
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9471:
URL: https://github.com/apache/pinot/pull/9471#discussion_r992591294
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransportFactory.java:
##########
@@ -80,6 +81,7 @@ public PinotControllerTransportFactory withConnectionProperties(Properties prope
DEFAULT_CONTROLLER_CONNECT_TIMEOUT_MS));
_handshakeTimeoutMs = Integer.parseInt(properties.getProperty("controllerHandshakeTimeoutMs",
DEFAULT_CONTROLLER_HANDSHAKE_TIMEOUT_MS));
+ _appId = properties.getProperty("appId", "");
Review Comment:
Same here, we want to set `null` when it is not configured
```suggestion
_appId = properties.getProperty("appId");
```
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/BrokerCache.java:
##########
@@ -104,6 +104,7 @@ public BrokerCache(Properties properties, String controllerUrl) {
DEFAULT_CONTROLLER_CONNECT_TIMEOUT_MS));
int handshakeTimeoutMs = Integer.parseInt(properties.getProperty("controllerHandshakeTimeoutMs",
DEFAULT_CONTROLLER_HANDSHAKE_TIMEOUT_MS));
+ String appId = properties.getProperty("appId", DEFAULT_CONTROLLER_HANDSHAKE_TIMEOUT_MS);
Review Comment:
```suggestion
String appId = properties.getProperty("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:
^^
##########
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:
The format is still not correct. Please update the IDE style setting and reformat
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransport.java:
##########
@@ -64,21 +66,26 @@ public PinotControllerTransport(Map<String, String> headers, String scheme,
builder.setReadTimeout(connectionTimeouts.getReadTimeoutMs())
.setConnectTimeout(connectionTimeouts.getConnectTimeoutMs())
.setHandshakeTimeout(connectionTimeouts.getHandshakeTimeoutMs())
- .setUserAgent(getUserAgentVersionFromClassPath())
+ .setUserAgent(getUserAgentVersionFromClassPath(appId))
.setEnabledProtocols(tlsProtocols.getEnabledProtocols().toArray(new String[0]));
_httpClient = Dsl.asyncHttpClient(builder.build());
}
- private String getUserAgentVersionFromClassPath() {
+ private String getUserAgentVersionFromClassPath(@Nullable String appId) {
Properties userAgentProperties = new Properties();
try {
userAgentProperties.load(PinotControllerTransport.class.getClassLoader()
.getResourceAsStream("version.properties"));
} catch (IOException e) {
LOGGER.warn("Unable to set user agent version");
}
- return userAgentProperties.getProperty("ua", "unknown");
+ String userAgentFromProperties = userAgentProperties.getProperty("ua", "unknown");
+ if (StringUtils.isNotEmpty(appId)) {
+ return appId.substring(0, Math.min(org.apache.pinot.client.utils.ConnectionUtils.APP_ID_MAX_CHARS,
Review Comment:
(minor) import `ConnectionUtils`
--
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
[GitHub] [pinot] rino-kadijk commented on pull request #9471: make first part of user agent header configurable
Posted by GitBox <gi...@apache.org>.
rino-kadijk commented on PR #9471:
URL: https://github.com/apache/pinot/pull/9471#issuecomment-1274243883
> Is there a constraint on the appId length? If so, we should fail it when user configure it with a too long value
I will take 256 chars.
https://stackoverflow.com/a/654997
--
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