You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tibrewalpratik17 (via GitHub)" <gi...@apache.org> on 2023/07/13 20:19:08 UTC
[GitHub] [pinot] tibrewalpratik17 opened a new pull request, #11103: Improve error message in QueryDispatcher
tibrewalpratik17 opened a new pull request, #11103:
URL: https://github.com/apache/pinot/pull/11103
In #11076 we were getting a very vague message in case of version conflicts.
`
java.io.IOException: Failed : HTTP error code : 500. Root Cause: <html><head><title>Grizzly 2.4.4</title><style><!--div.header {font-family:Tahoma,Arial,sans-serif;color:white;background-color:#003300;font-size:22px;-moz-border-radius-topleft: 10px;border-top-left-radius: 10px;-moz-border-radius-topright: 10px;border-top-right-radius: 10px;padding-left: 5px}div.body {font-family:Tahoma,Arial,sans-serif;color:black;background-color:#FFFFCC;font-size:16px;padding-top:10px;padding-bottom:10px;padding-left:10px}div.footer {font-family:Tahoma,Arial,sans-serif;color:white;background-color:#666633;font-size:14px;-moz-border-radius-bottomleft: 10px;border-bottom-left-radius: 10px;-moz-border-radius-bottomright: 10px;border-bottom-right-radius: 10px;padding-left: 5px}BODY {font-family:Tahoma,Arial,sans-serif;color:black;background-color:white;}B {font-family:Tahoma,Arial,sans-serif;color:black;}A {color : black;}HR {color : #999966;}--></style> </head><body><div class="header">Request fail
ed.</div><div class="body">Request failed.</div><div class="footer">Grizzly 2.4.4</div></body></html>
`
This patch gets the throwable message and propagates to the user. Sample error message:
`
[
{
"message": "QueryExecutionError:\njava.lang.RuntimeException: Error executing query: 'io.grpc.NameResolver$Args$Builder io.grpc.NameResolver$Args$Builder.setOverrideAuthority(java.lang.String)'\n\tat org.apache.pinot.query.service.dispatch.QueryDispatcher.submitAndReduce(QueryDispatcher.java:105)\n\tat org.apache.pinot.broker.requesthandler.MultiStageBrokerRequestHandler.handleRequest(MultiStageBrokerRequestHandler.java:218)\n\tat org.apache.pinot.broker.requesthandler.MultiStageBrokerRequestHandler.handleRequest(MultiStageBrokerRequestHandler.java:146)\n\tat org.apache.pinot.broker.requesthandler.BrokerRequestHandler.handleRequest(BrokerRequestHandler.java:47)",
"errorCode": 200
}
]
`
--
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] xiangfu0 merged pull request #11103: Improve error message in case of throwables in multistage
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #11103:
URL: https://github.com/apache/pinot/pull/11103
--
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] tibrewalpratik17 commented on a diff in pull request #11103: Improve error message in QueryDispatcher
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #11103:
URL: https://github.com/apache/pinot/pull/11103#discussion_r1263496675
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:
##########
@@ -101,6 +101,8 @@ public ResultTable submitAndReduce(long requestId, DispatchableSubPlan dispatcha
} catch (Exception e) {
cancel(requestId, dispatchableSubPlan);
throw new RuntimeException("Error executing query: " + ExplainPlanPlanVisitor.explain(dispatchableSubPlan), e);
+ } catch (Throwable t) {
Review Comment:
When I add `cancel(requestId, dispatchableSubPlan);` it tries to make a grpc call again and end up throwing the `NameResolver` error message. Do you suggest a nested try-catch around cancel as well then?
--
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] xiangfu0 commented on a diff in pull request #11103: Improve error message in QueryDispatcher
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11103:
URL: https://github.com/apache/pinot/pull/11103#discussion_r1263132905
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:
##########
@@ -101,6 +101,8 @@ public ResultTable submitAndReduce(long requestId, DispatchableSubPlan dispatcha
} catch (Exception e) {
cancel(requestId, dispatchableSubPlan);
throw new RuntimeException("Error executing query: " + ExplainPlanPlanVisitor.explain(dispatchableSubPlan), e);
+ } catch (Throwable t) {
Review Comment:
should you just merge Throwable with Exception?
--
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 #11103: Improve error message in QueryDispatcher
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11103:
URL: https://github.com/apache/pinot/pull/11103#issuecomment-1634911275
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11103?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11103](https://app.codecov.io/gh/apache/pinot/pull/11103?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (4c5314f) into [master](https://app.codecov.io/gh/apache/pinot/commit/1622a9e7d689537881826549a9323237ca7d7e44?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (1622a9e) will **increase** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11103 +/- ##
=========================================
Coverage 0.11% 0.11%
=========================================
Files 2203 2148 -55
Lines 118165 115646 -2519
Branches 17879 17573 -306
=========================================
Hits 137 137
+ Misses 118008 115489 -2519
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `?` | |
| integration1temurin20 | `?` | |
| integration2temurin11 | `?` | |
| integration2temurin17 | `?` | |
| unittests1temurin11 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `0.11% <0.00%> (ø)` | |
| 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/11103?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../pinot/query/service/dispatch/QueryDispatcher.java](https://app.codecov.io/gh/apache/pinot/pull/11103?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvc2VydmljZS9kaXNwYXRjaC9RdWVyeURpc3BhdGNoZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
... and [55 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11103/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 #11103: Improve error message in case of throwables in multistage
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11103:
URL: https://github.com/apache/pinot/pull/11103#discussion_r1264255147
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -218,9 +218,9 @@ private BrokerResponse handleRequest(long requestId, String query, @Nullable Sql
queryResults = _queryDispatcher.submitAndReduce(requestId, dispatchableSubPlan, _mailboxService,
_reducerScheduler,
queryTimeoutMs, sqlNodeAndOptions.getOptions(), stageIdStatsMap, traceEnabled);
- } catch (Exception e) {
- LOGGER.info("query execution failed", e);
- return new BrokerResponseNative(QueryException.getException(QueryException.QUERY_EXECUTION_ERROR, e));
+ } catch (Throwable t) {
+ LOGGER.error("query execution failed: " + t);
Review Comment:
```suggestion
LOGGER.error("query execution failed", t);
```
--
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] xiangfu0 commented on a diff in pull request #11103: Improve error message in QueryDispatcher
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11103:
URL: https://github.com/apache/pinot/pull/11103#discussion_r1263132905
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:
##########
@@ -101,6 +101,8 @@ public ResultTable submitAndReduce(long requestId, DispatchableSubPlan dispatcha
} catch (Exception e) {
cancel(requestId, dispatchableSubPlan);
throw new RuntimeException("Error executing query: " + ExplainPlanPlanVisitor.explain(dispatchableSubPlan), e);
+ } catch (Throwable t) {
Review Comment:
should you just merge Throwable with Exception?
Do you need to call `cancel(requestId, dispatchableSubPlan);` here as well?
--
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 #11103: Improve error message in QueryDispatcher
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11103:
URL: https://github.com/apache/pinot/pull/11103#discussion_r1263775378
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:
##########
@@ -101,6 +101,8 @@ public ResultTable submitAndReduce(long requestId, DispatchableSubPlan dispatcha
} catch (Exception e) {
cancel(requestId, dispatchableSubPlan);
throw new RuntimeException("Error executing query: " + ExplainPlanPlanVisitor.explain(dispatchableSubPlan), e);
+ } catch (Throwable t) {
Review Comment:
1. when a Throwable is caught, it usually indicates series issues and cannot be recovered.
2. netty handles the throwable thus the server didn't crash.
so here is my .02
1. don't need to explain the query, the throwable is serious enough that probably has nothing to do with the query itself.
2. log it and re-throw the throwable makes more sense to me instead of wrapping it back into a RTE
--
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] tibrewalpratik17 commented on pull request #11103: Improve error message in QueryDispatcher
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on PR #11103:
URL: https://github.com/apache/pinot/pull/11103#issuecomment-1635022420
> we shouldn't catch Throwable because Error usually means very serious problem and shouldn't be handled. Can you also print the Error:
throw new RuntimeException(ExplainPlanPlanVisitor.explain(dispatchableSubPlan), t);
The present error message doesn't tell anything and is really hard to debug so catching the throwable would add value in debugging these issues faster.
Here's a sample error message after the change suggested:
`
[
{
"message": "QueryExecutionError:\njava.lang.RuntimeException: [0]@127.0.0.1:60192 MAIL_RECEIVE(RANDOM_DISTRIBUTED)\n├── [1]@127.0.0.1:60213 MAIL_SEND(RANDOM_DISTRIBUTED)->{[0]@127.0.0.1@{60192,60192}|[0]} (Subtree Omitted)\n├── [1]@127.0.0.1:60200 MAIL_SEND(RANDOM_DISTRIBUTED)->{[0]@127.0.0.1@{60192,60192}|[0]} (Subtree Omitted)\n└── [1]@127.0.0.1:60207 MAIL_SEND(RANDOM_DISTRIBUTED)->{[0]@127.0.0.1@{60192,60192}|[0]}\n └── [1]@127.0.0.1:60207 SORT LIMIT 10\n...\nCaused by: java.lang.NoSuchMethodError: 'io.grpc.NameResolver$Args$Builder io.grpc.NameResolver$Args$Builder.setOverrideAuthority(java.lang.String)'\n\tat io.grpc.internal.ManagedChannelImpl.<init>(ManagedChannelImpl.java:659)\n\tat io.grpc.internal.ManagedChannelImplBuilder.build(ManagedChannelImplBuilder.java:631)\n\tat io.grpc.internal.AbstractManagedChannelImplBuilder.build(AbstractManagedChannelImplBuilder.java:297)\n\tat org.apache.pinot.query.service.dispatch.DispatchClient.<init>(Dispa
tchClient.java:46)",
"errorCode": 200
}
]
`
Do you think we should keep ExplainPlan in the errorResponse? Does it add a lot of information?
--
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] tibrewalpratik17 commented on a diff in pull request #11103: Improve error message in QueryDispatcher
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #11103:
URL: https://github.com/apache/pinot/pull/11103#discussion_r1264226251
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:
##########
@@ -101,6 +101,8 @@ public ResultTable submitAndReduce(long requestId, DispatchableSubPlan dispatcha
} catch (Exception e) {
cancel(requestId, dispatchableSubPlan);
throw new RuntimeException("Error executing query: " + ExplainPlanPlanVisitor.explain(dispatchableSubPlan), e);
+ } catch (Throwable t) {
Review Comment:
Planning to move this catch clause to `MultiStageBrokerRequestHandler` instead and merge it with the catch exception there.
--
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