You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "walterddr (via GitHub)" <gi...@apache.org> on 2023/07/25 18:07:11 UTC
[GitHub] [pinot] walterddr opened a new pull request, #11173: [multistage] fix error propagate physical planner
walterddr opened a new pull request, #11173:
URL: https://github.com/apache/pinot/pull/11173
see: #11167
--
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 #11173: [multistage] fix error propagate physical planner
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11173:
URL: https://github.com/apache/pinot/pull/11173#issuecomment-1650361762
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11173?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11173](https://app.codecov.io/gh/apache/pinot/pull/11173?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (29bdf1e) into [master](https://app.codecov.io/gh/apache/pinot/commit/ff6d527fbfe71cfa1afff601627b796ea59b168a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (ff6d527) will **increase** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11173 +/- ##
=========================================
Coverage 0.11% 0.11%
=========================================
Files 2213 2158 -55
Lines 118635 116162 -2473
Branches 17949 17653 -296
=========================================
Hits 137 137
+ Misses 118478 116005 -2473
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `?` | |
| integration1temurin20 | `?` | |
| integration2temurin20 | `?` | |
| unittests1temurin11 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `?` | |
| unittests2temurin17 | `0.11% <0.00%> (-0.01%)` | :arrow_down: |
| unittests2temurin20 | `0.11% <0.00%> (-0.01%)` | :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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files Changed](https://app.codecov.io/gh/apache/pinot/pull/11173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/query/service/QueryServer.java](https://app.codecov.io/gh/apache/pinot/pull/11173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvc2VydmljZS9RdWVyeVNlcnZlci5qYXZh) | `0.00% <0.00%> (ø)` | |
... and [61 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11173/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] walterddr commented on a diff in pull request #11173: [multistage] fix error propagate physical planner
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11173:
URL: https://github.com/apache/pinot/pull/11173#discussion_r1274242688
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/QueryServer.java:
##########
@@ -97,23 +100,45 @@ public void submit(Worker.QueryRequest request, StreamObserver<Worker.QueryRespo
Map<String, String> requestMetadataMap;
requestMetadataMap = request.getMetadataMap();
long requestId = Long.parseLong(requestMetadataMap.get(QueryConfig.KEY_OF_BROKER_REQUEST_ID));
+ long timeoutMs = Long.parseLong(requestMetadataMap.get(QueryConfig.KEY_OF_BROKER_REQUEST_TIMEOUT_MS));
+ long deadlineMs = System.currentTimeMillis() + timeoutMs;
try {
distributedStagePlans = QueryPlanSerDeUtils.deserializeStagePlan(request);
} catch (Exception e) {
LOGGER.error("Caught exception while deserializing the request: {}", requestId, e);
responseObserver.onError(Status.INVALID_ARGUMENT.withDescription("Bad request").withCause(e).asException());
return;
}
- // TODO: allow thrown exception to return back to broker in asynchronous manner.
+ BlockingQueue<String> stageSubmissionCallbacks = new LinkedBlockingQueue<>();
Review Comment:
sounds good.
--
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 #11173: [multistage] fix error propagate physical planner
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11173:
URL: https://github.com/apache/pinot/pull/11173#discussion_r1274123322
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/QueryServer.java:
##########
@@ -97,23 +100,45 @@ public void submit(Worker.QueryRequest request, StreamObserver<Worker.QueryRespo
Map<String, String> requestMetadataMap;
requestMetadataMap = request.getMetadataMap();
long requestId = Long.parseLong(requestMetadataMap.get(QueryConfig.KEY_OF_BROKER_REQUEST_ID));
+ long timeoutMs = Long.parseLong(requestMetadataMap.get(QueryConfig.KEY_OF_BROKER_REQUEST_TIMEOUT_MS));
+ long deadlineMs = System.currentTimeMillis() + timeoutMs;
try {
distributedStagePlans = QueryPlanSerDeUtils.deserializeStagePlan(request);
} catch (Exception e) {
LOGGER.error("Caught exception while deserializing the request: {}", requestId, e);
responseObserver.onError(Status.INVALID_ARGUMENT.withDescription("Bad request").withCause(e).asException());
return;
}
- // TODO: allow thrown exception to return back to broker in asynchronous manner.
+ BlockingQueue<String> stageSubmissionCallbacks = new LinkedBlockingQueue<>();
Review Comment:
We should track all the `Future`s and cancel them when query runs into problem. With `Future`, we can easily get whether it is done or throws exception. No need to use a separate blocking queue
--
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 #11173: [multistage] fix error propagate physical planner
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11173:
URL: https://github.com/apache/pinot/pull/11173#discussion_r1274120511
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/QueryServer.java:
##########
@@ -97,23 +100,45 @@ public void submit(Worker.QueryRequest request, StreamObserver<Worker.QueryRespo
Map<String, String> requestMetadataMap;
requestMetadataMap = request.getMetadataMap();
long requestId = Long.parseLong(requestMetadataMap.get(QueryConfig.KEY_OF_BROKER_REQUEST_ID));
+ long timeoutMs = Long.parseLong(requestMetadataMap.get(QueryConfig.KEY_OF_BROKER_REQUEST_TIMEOUT_MS));
+ long deadlineMs = System.currentTimeMillis() + timeoutMs;
try {
distributedStagePlans = QueryPlanSerDeUtils.deserializeStagePlan(request);
} catch (Exception e) {
LOGGER.error("Caught exception while deserializing the request: {}", requestId, e);
responseObserver.onError(Status.INVALID_ARGUMENT.withDescription("Bad request").withCause(e).asException());
return;
}
- // TODO: allow thrown exception to return back to broker in asynchronous manner.
+ BlockingQueue<String> stageSubmissionCallbacks = new LinkedBlockingQueue<>();
distributedStagePlans.forEach(distributedStagePlan -> _querySubmissionExecutorService.submit(() -> {
try {
_queryRunner.processQuery(distributedStagePlan, requestMetadataMap);
+ stageSubmissionCallbacks.offer(QueryConfig.KEY_OF_SERVER_RESPONSE_STATUS_OK);
} catch (Throwable t) {
LOGGER.error("Caught exception while compiling opChain for request: {}, stage: {}", requestId,
distributedStagePlan.getStageId(), t);
+ stageSubmissionCallbacks.offer(t.getMessage());
}
})
);
+
+ int successfulSubmissionCount = 0;
+ while (System.currentTimeMillis() < deadlineMs && successfulSubmissionCount < distributedStagePlans.size()) {
Review Comment:
@xiangfu0 @Jackie-Jiang please share your feedback on this one. This technically shouldn't matter much. But would like the see if there's any better handle
--
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 merged pull request #11173: [multistage] fix error propagate physical planner
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #11173:
URL: https://github.com/apache/pinot/pull/11173
--
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