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/20 23:44:43 UTC

[GitHub] [pinot] Jackie-Jiang opened a new pull request, #9443: Fix the bug of hybrid table request using the same request id

Jackie-Jiang opened a new pull request, #9443:
URL: https://github.com/apache/pinot/pull/9443

   Currently when querying a hybrid table, 2 requests (one for OFFLINE and one for REALTIME) are sent to the servers, but with the same request id. It can cause problem for 2 modules:
   - Tracing
   - Query cancellation
   
   This PR fixes the bug by using negative request id for REALTIME request in hybrid table.


-- 
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 #9443: Fix the bug of hybrid table request using the same request id

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #9443:
URL: https://github.com/apache/pinot/pull/9443#issuecomment-1289608147

   @jadami10 The problem is that for tracing and query cancellation, we use request id as the key to track the queries running on the server. For hybrid table, 2 queries are sent out, one for the OFFLINE table and one for the REALTIME table. If both tables are served on the same server, the 2 queries will override the entry for each other, and only one query can be tracked.
   You didn't run into this issue probably because the OFFLINE table and REALTIME table is never running on the same server.


-- 
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 #9443: Fix the bug of hybrid table request using the same request id

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #9443:
URL: https://github.com/apache/pinot/pull/9443#issuecomment-1253227909

   > Good catch! Do we also need the code change on canceling the request, since after this PR realtime servers may start to serve some queries with negative requestId?
   
   @jackjlli Yes, the query cancellation logic is also revised to cancel the correct request id on the server


-- 
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] jadami10 commented on pull request #9443: Fix the bug of hybrid table request using the same request id

Posted by GitBox <gi...@apache.org>.
jadami10 commented on PR #9443:
URL: https://github.com/apache/pinot/pull/9443#issuecomment-1289328378

   Is there a reason we needed to do it this way (negative ids)? Most log search technologies folks use will expect KV filter like `requestId=123`. This will by default filter out `requestId=-123` which I think is a pretty big regression. Why not add a new dimension to the log like `tableType`. That way folks know whether this was the realtime/offline side, we filter in by default, and now you can filter/stratify by offline/realtime as needed.


-- 
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] jadami10 commented on pull request #9443: Fix the bug of hybrid table request using the same request id

Posted by GitBox <gi...@apache.org>.
jadami10 commented on PR #9443:
URL: https://github.com/apache/pinot/pull/9443#issuecomment-1289613616

   That makes sense. In the case where we have 1 server serving both parts, does it/can it know which query is which part without passing more info from the broker? That way we could append `tableType` in the logs directly in the server rather than doing anything to the `requestId`.
   
   Is there actually a use case where someone wants to cancel a request id for just 1 part of the query? That feels strange since the broker will then return an exception anyway.


-- 
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 #9443: Fix the bug of hybrid table request using the same request id

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #9443:
URL: https://github.com/apache/pinot/pull/9443#issuecomment-1289791558

   @jadami10 Filed #9648 to handle this on the server side


-- 
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] klsince commented on a diff in pull request #9443: Fix the bug of hybrid table request using the same request id

Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9443:
URL: https://github.com/apache/pinot/pull/9443#discussion_r975902640


##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -190,28 +189,44 @@ public Map<Long, String> getRunningQueries() {
   Set<ServerInstance> getRunningServers(long requestId) {
     Preconditions.checkState(_queriesById != null, "Query cancellation is not enabled on broker");
     QueryServers queryServers = _queriesById.get(requestId);
-    return (queryServers == null) ? Collections.emptySet() : queryServers._servers;
+    Set<ServerInstance> runningServers = new HashSet<>();
+    if (queryServers._offlineRoutingTable != null) {
+      runningServers.addAll(queryServers._offlineRoutingTable.keySet());
+    }
+    if (queryServers._realtimeRoutingTable != null) {
+      runningServers.addAll(queryServers._realtimeRoutingTable.keySet());
+    }
+    return runningServers;
   }
 
   @Override
-  public boolean cancelQuery(long queryId, int timeoutMs, Executor executor, HttpConnectionManager connMgr,
+  public boolean cancelQuery(long requestId, int timeoutMs, Executor executor, HttpConnectionManager connMgr,
       Map<String, Integer> serverResponses)
       throws Exception {
     Preconditions.checkState(_queriesById != null, "Query cancellation is not enabled on broker");
-    QueryServers queryServers = _queriesById.get(queryId);
+    QueryServers queryServers = _queriesById.get(requestId);
     if (queryServers == null) {
       return false;
     }
-    String globalId = getGlobalQueryId(queryId);
     List<String> serverUrls = new ArrayList<>();
-    for (ServerInstance server : queryServers._servers) {
-      serverUrls.add(String.format("%s/query/%s", server.getAdminEndpoint(), globalId));
+    if (queryServers._offlineRoutingTable != null) {
+      for (ServerInstance server : queryServers._offlineRoutingTable.keySet()) {
+        serverUrls.add(String.format("%s/query/%s", server.getAdminEndpoint(), getGlobalQueryId(requestId)));
+      }
+    }
+    if (queryServers._realtimeRoutingTable != null) {
+      // NOTE: When the query is sent to both OFFLINE and REALTIME table, the REALTIME one has negative request id to
+      //       differentiate from the OFFLINE one
+      long realtimeRequestId = queryServers._offlineRoutingTable == null ? requestId : -requestId;

Review Comment:
   🆒 



##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -669,14 +678,22 @@ private BrokerResponseNative handleRequest(long requestId, String query,
       //       can always list the running queries and cancel query again until it ends. Just that such race
       //       condition makes cancel API less reliable. This should be rare as it assumes sending queries out to
       //       servers takes time, but will address later if needed.
-      QueryServers queryServers = _queriesById.computeIfAbsent(requestId, k -> new QueryServers(query));
+      _queriesById.put(requestId, new QueryServers(query, offlineRoutingTable, realtimeRoutingTable));
       LOGGER.debug("Keep track of running query: {}", requestId);
-      queryServers.addServers(offlineRoutingTable, realtimeRoutingTable);

Review Comment:
   one reason to use addServers() was to accumulate all servers involved when running a query, which might do some subqueries (thus recursively calling this handleRequest() method (where I assumed different routing tables might be created for some subqueries...) 



-- 
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] siddharthteotia merged pull request #9443: Fix the bug of hybrid table request using the same request id

Posted by GitBox <gi...@apache.org>.
siddharthteotia merged PR #9443:
URL: https://github.com/apache/pinot/pull/9443


-- 
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 #9443: Fix the bug of hybrid table request using the same request id

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9443:
URL: https://github.com/apache/pinot/pull/9443#discussion_r975907619


##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -669,14 +678,22 @@ private BrokerResponseNative handleRequest(long requestId, String query,
       //       can always list the running queries and cancel query again until it ends. Just that such race
       //       condition makes cancel API less reliable. This should be rare as it assumes sending queries out to
       //       servers takes time, but will address later if needed.
-      QueryServers queryServers = _queriesById.computeIfAbsent(requestId, k -> new QueryServers(query));
+      _queriesById.put(requestId, new QueryServers(query, offlineRoutingTable, realtimeRoutingTable));
       LOGGER.debug("Keep track of running query: {}", requestId);
-      queryServers.addServers(offlineRoutingTable, realtimeRoutingTable);

Review Comment:
   Yes, and that is the reason why I also moved the `_queriesById.remove(requestId)` into the `handleRequest()` to avoid this scenario



-- 
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 #9443: Fix the bug of hybrid table request using the same request id

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9443:
URL: https://github.com/apache/pinot/pull/9443#issuecomment-1253046015

   # [Codecov](https://codecov.io/gh/apache/pinot/pull/9443?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 [#9443](https://codecov.io/gh/apache/pinot/pull/9443?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b684292) into [master](https://codecov.io/gh/apache/pinot/commit/7be06af22c32433f6031fd7e1d29be1d2b96cb71?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7be06af) will **decrease** coverage by `43.77%`.
   > The diff coverage is `20.83%`.
   
   > :exclamation: Current head b684292 differs from pull request most recent head ac72549. Consider uploading reports for the commit ac72549 to get more accurate results
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #9443       +/-   ##
   =============================================
   - Coverage     69.76%   25.99%   -43.78%     
   + Complexity     5098       44     -5054     
   =============================================
     Files          1890     1878       -12     
     Lines        100934   100590      -344     
     Branches      15347    15313       -34     
   =============================================
   - Hits          70420    26149    -44271     
   - Misses        25541    71826    +46285     
   + Partials       4973     2615     -2358     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `25.99% <20.83%> (+0.09%)` | :arrow_up: |
   | integration2 | `?` | |
   | unittests1 | `?` | |
   | unittests2 | `?` | |
   
   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/9443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...roker/requesthandler/GrpcBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/9443/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvR3JwY0Jyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `0.00% <0.00%> (-86.96%)` | :arrow_down: |
   | [...he/pinot/common/utils/grpc/GrpcRequestBuilder.java](https://codecov.io/gh/apache/pinot/pull/9443/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUmVxdWVzdEJ1aWxkZXIuamF2YQ==) | `0.00% <0.00%> (-78.79%)` | :arrow_down: |
   | [...che/pinot/core/query/scheduler/QueryScheduler.java](https://codecov.io/gh/apache/pinot/pull/9443/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zY2hlZHVsZXIvUXVlcnlTY2hlZHVsZXIuamF2YQ==) | `81.20% <ø> (-4.70%)` | :arrow_down: |
   | [...e/pinot/core/transport/InstanceRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/9443/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvSW5zdGFuY2VSZXF1ZXN0SGFuZGxlci5qYXZh) | `39.84% <0.00%> (-25.00%)` | :arrow_down: |
   | [...c/main/java/org/apache/pinot/spi/trace/Tracer.java](https://codecov.io/gh/apache/pinot/pull/9443/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvVHJhY2VyLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/9443/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `56.09% <15.15%> (-14.16%)` | :arrow_down: |
   | [...che/pinot/core/query/reduce/BaseReduceService.java](https://codecov.io/gh/apache/pinot/pull/9443/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvQmFzZVJlZHVjZVNlcnZpY2UuamF2YQ==) | `95.31% <100.00%> (ø)` | |
   | [...a/org/apache/pinot/core/transport/QueryRouter.java](https://codecov.io/gh/apache/pinot/pull/9443/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvUXVlcnlSb3V0ZXIuamF2YQ==) | `72.61% <100.00%> (-9.09%)` | :arrow_down: |
   | [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/9443/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/9443/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [1373 more](https://codecov.io/gh/apache/pinot/pull/9443/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] klsince commented on a diff in pull request #9443: Fix the bug of hybrid table request using the same request id

Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9443:
URL: https://github.com/apache/pinot/pull/9443#discussion_r975907845


##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -669,14 +678,22 @@ private BrokerResponseNative handleRequest(long requestId, String query,
       //       can always list the running queries and cancel query again until it ends. Just that such race
       //       condition makes cancel API less reliable. This should be rare as it assumes sending queries out to
       //       servers takes time, but will address later if needed.
-      QueryServers queryServers = _queriesById.computeIfAbsent(requestId, k -> new QueryServers(query));
+      _queriesById.put(requestId, new QueryServers(query, offlineRoutingTable, realtimeRoutingTable));
       LOGGER.debug("Keep track of running query: {}", requestId);
-      queryServers.addServers(offlineRoutingTable, realtimeRoutingTable);

Review Comment:
   Make sense!



-- 
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 #9443: Fix the bug of hybrid table request using the same request id

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #9443:
URL: https://github.com/apache/pinot/pull/9443#issuecomment-1289396873

   @jadami10 Good point. The reason why we choose to fix the issue by using negative id is because that won't change the transport object between brokers and servers, which is backward-incompatible change. On the server side, we may check whether the request-id is positive and log it accordingly.


-- 
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] jadami10 commented on pull request #9443: Fix the bug of hybrid table request using the same request id

Posted by GitBox <gi...@apache.org>.
jadami10 commented on PR #9443:
URL: https://github.com/apache/pinot/pull/9443#issuecomment-1289440000

   > that won't change the transport object between brokers and servers
   
   does that matter since it was already working before? The description states the components it's trying to fix but not really what was broken, so it's a little tough for me to understand what the problem is in the first place.


-- 
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 #9443: Fix the bug of hybrid table request using the same request id

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #9443:
URL: https://github.com/apache/pinot/pull/9443#issuecomment-1289620322

   That is possible by checking the table name. Let me think if we can solve it on server itself.
   
   Currently we don't allow cancelling only one request. From endpoint perspective, it will be the same, where user still always give the positive request id, and broker automatically re-write it to negative one if necessary. The issue is that the logger might log the negative request id on the server side.


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