make first part of user agent header configurable #9471

rino-kadijk · 2022-09-27T09:44:28Z

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

gortiz

LGTM

Jackie-Jiang

LGTM in general. Minor comments

Jackie-Jiang · 2022-09-28T01:25:02Z

...not-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,


(format) Please follow the Pinot Style. Same for other changes

Annotate appId as @Nullable

The format is still not correct. Please update the IDE style setting and reformat

Jackie-Jiang · 2022-09-28T01:27:41Z

...a-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 = "";


Suggest setting it to null if not available

Jackie-Jiang · 2022-09-28T01:27:59Z

...a-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", "");


Set it to null if not available

Jackie-Jiang · 2022-09-28T01:28:12Z

...t-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) {


Suggested change

public static String getUserAgentVersionFromClassPath(String userAgentKey, String appId) {

public static String getUserAgentVersionFromClassPath(String userAgentKey, @Nullable String appId) {

Jackie-Jiang · 2022-09-28T01:28:37Z

...t-clients/pinot-java-client/src/main/java/org/apache/pinot/client/utils/ConnectionUtils.java

    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) {


(nit) StringUtils.isNotEmpty(appId)

Jackie-Jiang · 2022-09-28T01:30:12Z

...t-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,


Same comments as in JsonAsyncHttpPinotClientTransport

Jackie-Jiang · 2022-09-28T01:30:26Z

...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 = "";


Same comments as in JsonAsyncHttpPinotClientTransportFactory

codecov-commenter · 2022-09-28T02:19:18Z

Codecov Report

Merging #9471 (4b60900) into master (83b7f15) will increase coverage by 0.01%.
The diff coverage is 68.18%.

@@             Coverage Diff              @@
##             master    #9471      +/-   ##
============================================
+ Coverage     69.89%   69.90%   +0.01%     
- Complexity     4742     4818      +76     
============================================
  Files          1910     1910              
  Lines        101787   101798      +11     
  Branches      15445    15447       +2     
============================================
+ Hits          71139    71161      +22     
+ Misses        25628    25604      -24     
- Partials       5020     5033      +13

Flag	Coverage Δ
integration1	`26.09% <63.63%> (+0.05%)`	⬆️
integration2	`24.81% <63.63%> (+0.11%)`	⬆️
unittests1	`67.14% <ø> (-0.01%)`	⬇️
unittests2	`15.49% <50.00%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...main/java/org/apache/pinot/client/BrokerCache.java	`0.00% <0.00%> (ø)`
...inot/client/JsonAsyncHttpPinotClientTransport.java	`45.78% <50.00%> (ø)`
...org/apache/pinot/client/utils/ConnectionUtils.java	`50.00% <50.00%> (-3.34%)`	⬇️
...ot/client/controller/PinotControllerTransport.java	`49.18% <83.33%> (+0.90%)`	⬆️
...ient/JsonAsyncHttpPinotClientTransportFactory.java	`70.58% <100.00%> (+1.83%)`	⬆️
...nt/controller/PinotControllerTransportFactory.java	`82.14% <100.00%> (+1.37%)`	⬆️
...core/operator/docvalsets/TransformBlockValSet.java	`61.16% <0.00%> (-2.92%)`	⬇️
.../pinot/core/query/scheduler/PriorityScheduler.java	`80.55% <0.00%> (-2.78%)`	⬇️
...troller/helix/core/retention/RetentionManager.java	`79.03% <0.00%> (-2.42%)`	⬇️
...pache/pinot/minion/event/MinionEventObservers.java	`89.65% <0.00%> (-1.73%)`	⬇️
... and 23 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

rino-kadijk · 2022-09-28T14:07:45Z

LGTM in general. Minor comments

will fix, should I trim the appId to 32 characters?

Jackie-Jiang · 2022-09-29T20:55:01Z

Is there a constraint on the appId length? If so, we should fail it when user configure it with a too long value

rino-kadijk · 2022-10-11T07:50:29Z

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

…ded test

Jackie-Jiang · 2022-10-11T17:10:07Z

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);


Suggested change

String appId = properties.getProperty("appId", DEFAULT_CONTROLLER_HANDSHAKE_TIMEOUT_MS);

String appId = properties.getProperty("appId");

Jackie-Jiang · 2022-10-11T17:10:47Z

...not-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,


The format is still not correct. Please update the IDE style setting and reformat

Jackie-Jiang · 2022-10-11T17:11:20Z

...a-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", "");


Jackie-Jiang · 2022-10-11T17:12:58Z

...t-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransport.java

-    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,


(minor) import ConnectionUtils

[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.

Jackie-Jiang · 2022-10-11T17:13:58Z

...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", "");


Same here, we want to set null when it is not configured

Suggested change

_appId = properties.getProperty("appId", "");

_appId = properties.getProperty("appId");

Jackie-Jiang

Please take a look at the failed tests

Jackie-Jiang · 2022-10-17T21:10:11Z

...not-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,


Please reformat this file

rino-kadijk · 2022-10-18T09:58:10Z

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)

nor this codepath:

java.lang.IllegalArgumentException: Cannot replace segment with different name for table: {}, old segment: {}, new segment: {} [testTable_REALTIME, testTable__0__1__20221017T1608Z, testTable__0__1__20221017T1609Z]
[6065](https://github.com/apache/pinot/actions/runs/3266516098/jobs/5370356271#step:5:6066)
	at com.google.common.base.Preconditions.checkArgument(Preconditions.java:399)
[6066](https://github.com/apache/pinot/actions/runs/3266516098/jobs/5370356271#step:5:6067)
	at org.apache.pinot.segment.local.upsert.BasePartitionUpsertMetadataManager.replaceSegment(BasePartitionUpsertMetadataManager.java:164)
[6067](https://github.com/apache/pinot/actions/runs/3266516098/jobs/5370356271#step:5:6068)
	at org.apache.pinot.segment.local.upsert.ConcurrentMapPartitionUpsertMetadataManagerTest.verifyAddReplaceRemoveSegment(ConcurrentMapPartitionUpsertMetadataManagerTest.java:147)
[6068](https://github.com/apache/pinot/actions/runs/3266516098/jobs/5370356271#step:5:6069)
	at org.apache.pinot.segment.local.upsert.ConcurrentMapPartitionUpsertMetadataManagerTest.testAddReplaceRemoveSegment(ConcurrentMapPartitionUpsertMetadataManagerTest.java:60)

make first part of user agent header configurable

4b60900

gortiz approved these changes Sep 27, 2022

View reviewed changes

Jackie-Jiang reviewed Sep 28, 2022

View reviewed changes

rino-kadijk added 2 commits October 11, 2022 10:12

max 256 chars for UA string, adjusted to Pinot Style, Nullable and ad…

68e0b34

…ded test

merge origin

fa71a57

Jackie-Jiang reviewed Oct 11, 2022

View reviewed changes

set appId to null and apply stylesheet

c9b7fa2

Jackie-Jiang reviewed Oct 17, 2022

View reviewed changes

reformat file

c904335

Jackie-Jiang approved these changes Oct 19, 2022

View reviewed changes

Jackie-Jiang added the enhancement label Oct 19, 2022

Jackie-Jiang merged commit 3e1aba3 into apache:master Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make first part of user agent header configurable #9471

make first part of user agent header configurable #9471

rino-kadijk commented Sep 27, 2022

gortiz left a comment

Jackie-Jiang left a comment

Jackie-Jiang Sep 28, 2022

Jackie-Jiang Sep 28, 2022

Jackie-Jiang Oct 11, 2022

Jackie-Jiang Sep 28, 2022

Jackie-Jiang Sep 28, 2022

Jackie-Jiang Oct 11, 2022

Jackie-Jiang Sep 28, 2022

Jackie-Jiang Sep 28, 2022

Jackie-Jiang Sep 28, 2022

Jackie-Jiang Sep 28, 2022

codecov-commenter commented Sep 28, 2022 •

edited

rino-kadijk commented Sep 28, 2022

Jackie-Jiang commented Sep 29, 2022

rino-kadijk commented Oct 11, 2022

Jackie-Jiang Oct 11, 2022

Jackie-Jiang Oct 11, 2022

Jackie-Jiang Oct 11, 2022

Jackie-Jiang Oct 11, 2022

rino-kadijk Oct 17, 2022

Jackie-Jiang Oct 11, 2022

Jackie-Jiang left a comment

Jackie-Jiang Oct 17, 2022

rino-kadijk commented Oct 18, 2022 •

edited

	public static String getUserAgentVersionFromClassPath(String userAgentKey, String appId) {
	public static String getUserAgentVersionFromClassPath(String userAgentKey, @Nullable String appId) {

	String appId = properties.getProperty("appId", DEFAULT_CONTROLLER_HANDSHAKE_TIMEOUT_MS);
	String appId = properties.getProperty("appId");

	_appId = properties.getProperty("appId", "");
	_appId = properties.getProperty("appId");

make first part of user agent header configurable #9471

make first part of user agent header configurable #9471

Conversation

rino-kadijk commented Sep 27, 2022

gortiz left a comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 28, 2022 • edited

Codecov Report

rino-kadijk commented Sep 28, 2022

Jackie-Jiang commented Sep 29, 2022

rino-kadijk commented Oct 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rino-kadijk commented Oct 18, 2022 • edited

codecov-commenter commented Sep 28, 2022 •

edited

rino-kadijk commented Oct 18, 2022 •

edited