You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "mingmxu (via GitHub)" <gi...@apache.org> on 2023/06/19 08:01:51 UTC

[GitHub] [pinot] mingmxu opened a new pull request, #10943: [#10884][feature] add requestId for BrokerResponse in pinot-broker and java-client

mingmxu opened a new pull request, #10943:
URL: https://github.com/apache/pinot/pull/10943

   # issue 
   https://github.com/apache/pinot/issues/10884
   
   # what's the problem?
   * `requestId` is available only for response with v2 engine;
   * `requestId` is not exposed in java-client;
   
   # What's the change?
   * `getRequestId()` and `setRequestId()` is moved from `BrokerResponseNativeV2` to `org.apache.pinot.common.response.BrokerResponse`;
   * return `requestId` in `BaseBrokerRequestHandler#handleRequest()` which can benefit both `SingleConnectionBrokerRequestHandler` and `GrpcBrokerRequestHandler`;
   * parse `requestId` in `org.apache.pinot.client.BrokerResponse`, the field is set to optional for client/server compatibility; 
   
   # Note
   * `BaseBrokerRequestHandler`(v1 engine) could generate duplicate requestId crossing brokers, leave it in another PR if prefer to fix;
   * there're still two implementations of `BrokerResponse`, also leave it to another PR to limit change scope; 
   


-- 
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] walterddr commented on a diff in pull request #10943: [#10884][feature] add requestId for BrokerResponse in pinot-broker and java-client

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10943:
URL: https://github.com/apache/pinot/pull/10943#discussion_r1239097091


##########
pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseStats.java:
##########
@@ -33,7 +33,7 @@
 //  same metadataKey
 // TODO: Replace member fields with a simple map of <MetadataKey, Object>
 // TODO: Add a subStat field, stage level subStats will contain each operator stats
-@JsonPropertyOrder({"exceptions", "numBlocks", "numRows", "stageExecutionTimeMs", "stageExecutionUnit",
+@JsonPropertyOrder({"requestId", "exceptions", "numBlocks", "numRows", "stageExecutionTimeMs", "stageExecutionUnit",

Review Comment:
   requestID generated in v1 engine doesn't include a hashed encoding of the broker address. so essentially this could cause confusion (e.g. requestID starts with 0 and you might get another 0 when hitting a different broker)
   
   see: #10789 for more info



-- 
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 #10943: [#10884][feature] add requestId for BrokerResponse in pinot-broker and java-client

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10943:
URL: https://github.com/apache/pinot/pull/10943#issuecomment-1603121571

   ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10943?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#10943](https://app.codecov.io/gh/apache/pinot/pull/10943?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6cd2a51) into [master](https://app.codecov.io/gh/apache/pinot/commit/aea890a5ee715bf06bf7dd1643a4e8ced3db67b9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (aea890a) will **increase** coverage by `0.11%`.
   > The diff coverage is `0.00%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10943      +/-   ##
   ==========================================
   + Coverage    0.00%    0.11%   +0.11%     
   ==========================================
     Files        2173     2135      -38     
     Lines      117496   115443    -2053     
     Branches    17779    17538     -241     
   ==========================================
   + Hits            0      137     +137     
   + Misses     117496   115286    -2210     
   - Partials        0       20      +20     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1temurin11 | `?` | |
   | integration1temurin17 | `?` | |
   | integration1temurin20 | `?` | |
   | integration2temurin11 | `?` | |
   | integration2temurin17 | `?` | |
   | integration2temurin20 | `?` | |
   | unittests1temurin17 | `?` | |
   | unittests1temurin20 | `?` | |
   | unittests2temurin17 | `0.11% <0.00%> (?)` | |
   | unittests2temurin20 | `0.11% <0.00%> (?)` | |
   
   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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10943?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://app.codecov.io/gh/apache/pinot/pull/10943?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...n/java/org/apache/pinot/client/BrokerResponse.java](https://app.codecov.io/gh/apache/pinot/pull/10943?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Jyb2tlclJlc3BvbnNlLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...t/common/response/broker/BrokerResponseNative.java](https://app.codecov.io/gh/apache/pinot/pull/10943?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzcG9uc2UvYnJva2VyL0Jyb2tlclJlc3BvbnNlTmF0aXZlLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...common/response/broker/BrokerResponseNativeV2.java](https://app.codecov.io/gh/apache/pinot/pull/10943?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzcG9uc2UvYnJva2VyL0Jyb2tlclJlc3BvbnNlTmF0aXZlVjIuamF2YQ==) | `0.00% <ø> (ø)` | |
   | [...ot/common/response/broker/BrokerResponseStats.java](https://app.codecov.io/gh/apache/pinot/pull/10943?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzcG9uc2UvYnJva2VyL0Jyb2tlclJlc3BvbnNlU3RhdHMuamF2YQ==) | `0.00% <ø> (ø)` | |
   
   ... and [73 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10943/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   :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=apache)
   


-- 
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] mingmxu commented on pull request #10943: [#10884][feature] add requestId for BrokerResponse in pinot-broker and java-client

Posted by "mingmxu (via GitHub)" <gi...@apache.org>.
mingmxu commented on PR #10943:
URL: https://github.com/apache/pinot/pull/10943#issuecomment-1597463893

   cc @Jackie-Jiang 


-- 
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] mingmxu commented on a diff in pull request #10943: [#10884][feature] add requestId for BrokerResponse in pinot-broker and java-client

Posted by "mingmxu (via GitHub)" <gi...@apache.org>.
mingmxu commented on code in PR #10943:
URL: https://github.com/apache/pinot/pull/10943#discussion_r1239291556


##########
pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseStats.java:
##########
@@ -33,7 +33,7 @@
 //  same metadataKey
 // TODO: Replace member fields with a simple map of <MetadataKey, Object>
 // TODO: Add a subStat field, stage level subStats will contain each operator stats
-@JsonPropertyOrder({"exceptions", "numBlocks", "numRows", "stageExecutionTimeMs", "stageExecutionUnit",
+@JsonPropertyOrder({"requestId", "exceptions", "numBlocks", "numRows", "stageExecutionTimeMs", "stageExecutionUnit",

Review Comment:
   Yeah that's a problem, I'll submit another PR to reuse `MultiStageRequestIdGenerator` in v1 engine. 



-- 
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] mingmxu commented on pull request #10943: [#10884][feature] add requestId for BrokerResponse in pinot-broker and java-client

Posted by "mingmxu (via GitHub)" <gi...@apache.org>.
mingmxu commented on PR #10943:
URL: https://github.com/apache/pinot/pull/10943#issuecomment-1603078045

   Thanks @Jackie-Jiang , updated to address comment and fix conflict. 


-- 
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] walterddr commented on a diff in pull request #10943: [#10884][feature] add requestId for BrokerResponse in pinot-broker and java-client

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10943:
URL: https://github.com/apache/pinot/pull/10943#discussion_r1239097091


##########
pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseStats.java:
##########
@@ -33,7 +33,7 @@
 //  same metadataKey
 // TODO: Replace member fields with a simple map of <MetadataKey, Object>
 // TODO: Add a subStat field, stage level subStats will contain each operator stats
-@JsonPropertyOrder({"exceptions", "numBlocks", "numRows", "stageExecutionTimeMs", "stageExecutionUnit",
+@JsonPropertyOrder({"requestId", "exceptions", "numBlocks", "numRows", "stageExecutionTimeMs", "stageExecutionUnit",

Review Comment:
   requestID generated in v1 engine doesn't include a hashed encoding of the broker address. so essentially this could cause confusion (e.g. requestID starts with 0 from one broker, but you might get another 0 when hitting a different broker a bit later)
   
   see: #10789 for more info



-- 
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 #10943: [#10884][feature] add requestId for BrokerResponse in pinot-broker and java-client

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10943:
URL: https://github.com/apache/pinot/pull/10943


-- 
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 #10943: [#10884][feature] add requestId for BrokerResponse in pinot-broker and java-client

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10943:
URL: https://github.com/apache/pinot/pull/10943#discussion_r1237924010


##########
pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseNative.java:
##########
@@ -557,4 +558,16 @@ public void addToExceptions(QueryProcessingException processingException) {
   public int getExceptionsSize() {
     return _processingExceptions.size();
   }
+

Review Comment:
   (nit) Extra empty line



##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/BrokerResponse.java:
##########
@@ -35,6 +36,7 @@ private BrokerResponse() {
   }
 
   private BrokerResponse(JsonNode brokerResponse) {
+    _requestId = brokerResponse.get("requestId") != null ? brokerResponse.get("requestId").asText() : "-1";

Review Comment:
   Consider putting "unknown"?



##########
pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseNative.java:
##########
@@ -557,4 +558,16 @@ public void addToExceptions(QueryProcessingException processingException) {
   public int getExceptionsSize() {
     return _processingExceptions.size();
   }
+
+
+  @JsonProperty("requestId")
+  @Override
+  public String getRequestId() {
+    return _requestId;
+  }
+
+  @Override

Review Comment:
   Also annotate (JsonProperty) the setter? (Although seems the annotation is redundant)



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