You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2023/08/25 09:13:50 UTC
[GitHub] [pinot] gortiz opened a new pull request, #11440: Return better error info when no servers instances can be found in mu…
gortiz opened a new pull request, #11440:
URL: https://github.com/apache/pinot/pull/11440
This PR changes the error when there are no available servers for worker assignment.
The new message is:
```json
[
{
"message": "ServerInstanceMissingError:\nFailed to find enabled fully replicated server for table: tableName, partition: i in table: tableName",
"errorCode": 715
}
]
```
Without this PR, the error show was something like
```json
[
{
"message": "SQLParsingError:\nError composing query plan for: SET timeoutMs = 600000;\n\nSELECT opsgenie_team.name, innerTable.team_id AS 'Team ID', innerTable.cnt AS 'Count of all Alert ID'\nFROM opsgenie_team\nJOIN(\nSELECT /*+ joinOptions(join_strategy='dynamic_broadcast') */\nAlertResponderMapping.team_id as team_id, COUNT(*) AS cnt\nFROM opsgenie_alert_responder_mapping /*+ tableOptions(partition_key='alert_id', partition_size='16') */ AlertResponderMapping\nWHERE AlertResponderMapping.alert_id IN (\nSELECT alert_id\nFROM opsgenie_alert /*+ tableOptions(partition_key='alert_id', partition_size='16') */\nWHERE workspace_id = 'workspace_0' AND created_at_month = datetrunc('month', now())\n)\nGROUP BY AlertResponderMapping.team_id\n) innerTable ON opsgenie_team.team_id = innerTable.team_id\nORDER BY innerTable.cnt DESC, innerTable.team_id ASC\nLIMIT 100000;\norg.apache.pinot.query.QueryEnvironment.planQuery(QueryEnvironment.java:182)\norg.apache.pinot.broker.requesthandler.M
ultiStageBrokerRequestHandler.handleRequest(MultiStageBrokerRequestHandler.java:137)\norg.apache.pinot.broker.requesthandler.BaseBrokerRequestHandler.handleRequest(BaseBrokerRequestHandler.java:263)\norg.apache.pinot.broker.requesthandler.BrokerRequestHandler.handleRequest(BrokerRequestHandler.java:48)\nFailed to find enabled fully replicated server for table: opsgenie_alert, partition: 0 in table: %s\nai.startree.shaded.com.google.common.base.Preconditions.checkState(Preconditions.java:802)\norg.apache.pinot.query.routing.WorkerManager.assignWorkersToPartitionedLeafFragment(WorkerManager.java:231)\norg.apache.pinot.query.routing.WorkerManager.assignWorkersToLeafFragment(WorkerManager.java:119)\norg.apache.pinot.query.routing.WorkerManager.assignWorkersToNonRootFragment(WorkerManager.java:85)\n",
"errorCode": 150
}
]
```
In order to correctly react to this error, I've modified the code a bit to throw `ProcessingException` as soon as we find the issue and just rethrow from the stack. I'm not sure if this is the most correct solution. It seems that `ProcessingException` is coupled with the wire protocol, so maybe we would prefer to use a protocol generic exception in this case, but I didn't want to create a very large PR.
IMHO the current way to propagate the exceptions relays too much on generic runtime exceptions (like `IllegalArgumentException`, `IllegalStateException` or `RuntimeException` itself), which makes it very difficult to know whether we can send the error message to the user (like in this case) or whether we need to wrap the exception and therefore associate a generic (and possible incorrect, like in this case) error code.
Feel free to suggest other alternatives.
cc: @xiangfu0 @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] gortiz commented on a diff in pull request #11440: Return better error info when no servers instances can be found in mu…
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #11440:
URL: https://github.com/apache/pinot/pull/11440#discussion_r1305413347
##########
pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java:
##########
@@ -180,6 +185,20 @@ public static ProcessingException getException(ProcessingException processingExc
return copiedProcessingException;
}
+ public static void decorateException(ProcessingException processingException) {
+ String errorType;
+ switch (processingException.getErrorCode()) {
+ case SERVER_INSTANCE_MISSING_ERROR_CODE:
+ errorType = "ServerInstanceMissingError";
+ break;
+ // TODO: add more branches
Review Comment:
I don't like this idea (used above in `getException`) where we have a base exception with the message and then we do a deep copy. IMHO it would be better to just have an exception class for each ProcessingException or a builder method that takes the error code and the message and does something like this method, creating the exception with the `errorType` prefix associated with the given error code.
I'm not using this method as I would like because I'm following the existent pattern where the exception is first thrown without the `errorType` prefix and then it is decorated.
--
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 #11440: Return better error info when no servers instances can be found in mu…
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #11440:
URL: https://github.com/apache/pinot/pull/11440
--
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] gortiz commented on pull request #11440: Return better error info when no servers instances can be found in mu…
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #11440:
URL: https://github.com/apache/pinot/pull/11440#issuecomment-1706035723
I've changed the whole code to follow the approach suggested by @Jackie-Jiang. IMHO this is worse, but given that no one else has spoken, I guess it is not an important matter
--
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] gortiz commented on pull request #11440: Return better error info when no servers instances can be found in mu…
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #11440:
URL: https://github.com/apache/pinot/pull/11440#issuecomment-1707774025
I've changed that test to use error codes, which is better (mainly because in case of failure we actually get the actual value)
--
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] gortiz commented on a diff in pull request #11440: Return better error info when no servers instances can be found in mu…
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #11440:
URL: https://github.com/apache/pinot/pull/11440#discussion_r1305410184
##########
pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java:
##########
@@ -152,7 +156,7 @@ public static void setMaxLinesOfStackTrace(int maxLinesOfStackTracePerFrame) {
QUERY_SCHEDULING_TIMEOUT_ERROR.setMessage("QuerySchedulingTimeoutError");
SERVER_RESOURCE_LIMIT_EXCEEDED_ERROR.setMessage("ServerResourceLimitExceededError");
EXECUTION_TIMEOUT_ERROR.setMessage("ExecutionTimeoutError");
- DATA_TABLE_DESERIALIZATION_ERROR.setMessage("DataTableSerializationError");
+ DATA_TABLE_SERIALIZATION_ERROR.setMessage("DataTableSerializationError");
Review Comment:
I've also changed this because it seems like an error. `DATA_TABLE_DESERIALIZATION_ERROR` was in fact set 2 lines below
--
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] gortiz commented on pull request #11440: Return better error info when no servers instances can be found in mu…
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #11440:
URL: https://github.com/apache/pinot/pull/11440#issuecomment-1695433829
@Jackie-Jiang I don't understand how do you propose to make the message more crisp. How do you know, in pinot-`MultiStageBrokerRequestHandler` (or `QueryEnvironment`) the actual error reason? The only way I think we have is to parse the message of the exception (looking for something that starts with `Failed to find enabled fully replicated server for table:`), which seems like a very bad idea. Is there something I'm missing?
Remember that we don't just want to change the error message, we also want to provide a more specific error code. In my example we return 715 instead of the generic 150.
Do you propose to include the exception trace or not?
--
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 pull request #11440: Return better error info when no servers instances can be found in mu…
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #11440:
URL: https://github.com/apache/pinot/pull/11440#issuecomment-1706911966
Please fix the test in `org.apache.pinot.integration.tests.custom.JsonPathTest`.`testFailedQuery()`
--
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 pull request #11440: Return better error info when no servers instances can be found in mu…
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #11440:
URL: https://github.com/apache/pinot/pull/11440#issuecomment-1707699367
This is the diff:
```
diff --git a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java
index f6a2345a62..4dba3d888f 100644
--- a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java
+++ b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java
@@ -450,7 +450,8 @@ public class MultiStageEngineIntegrationTest extends BaseClusterIntegrationTestS
// invalid argument
sqlQuery = "SELECT toBase64('hello!') FROM mytable";
response = postQuery(sqlQuery);
- assertTrue(response.get("exceptions").get(0).get("message").toString().contains("SQLParsingError"));
+ assertTrue(
+ response.get("exceptions").get(0).get("message").toString().contains("value hello! does not match type class"));
// invalid argument
sqlQuery = "SELECT fromBase64('hello!') FROM mytable";
```
--
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] gortiz commented on a diff in pull request #11440: Return better error info when no servers instances can be found in mu…
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #11440:
URL: https://github.com/apache/pinot/pull/11440#discussion_r1315443488
##########
pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java:
##########
@@ -152,7 +156,7 @@ public static void setMaxLinesOfStackTrace(int maxLinesOfStackTracePerFrame) {
QUERY_SCHEDULING_TIMEOUT_ERROR.setMessage("QuerySchedulingTimeoutError");
SERVER_RESOURCE_LIMIT_EXCEEDED_ERROR.setMessage("ServerResourceLimitExceededError");
EXECUTION_TIMEOUT_ERROR.setMessage("ExecutionTimeoutError");
- DATA_TABLE_DESERIALIZATION_ERROR.setMessage("DataTableSerializationError");
+ DATA_TABLE_SERIALIZATION_ERROR.setMessage("DataTableSerializationError");
Review Comment:
Already fixed by @Jackie-Jiang in d8d1e92cb9fd252b00b44f303c77e22592c47ecd
--
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] gortiz commented on a diff in pull request #11440: Return better error info when no servers instances can be found in mu…
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #11440:
URL: https://github.com/apache/pinot/pull/11440#discussion_r1305415168
##########
pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java:
##########
@@ -180,6 +185,20 @@ public static ProcessingException getException(ProcessingException processingExc
return copiedProcessingException;
}
+ public static void decorateException(ProcessingException processingException) {
+ String errorType;
+ switch (processingException.getErrorCode()) {
+ case SERVER_INSTANCE_MISSING_ERROR_CODE:
+ errorType = "ServerInstanceMissingError";
+ break;
+ // TODO: add more branches
Review Comment:
I don't like this idea (used above in `getException`) where we have a base exception with the message and then we do a deep copy. IMHO it would be better to just have an exception class for each ProcessingException or a builder method that takes the error code and the message and does something like this method, creating the exception with the `errorType` prefix associated with the given error code.
I'm not using this method as I would like because I'm following the existent pattern where the exception is first thrown without the `errorType` prefix and then it is decorated.
In case we want to continue using the pattern in this PR, we just need to add here the prefix for each error code (or use an String[] or a map from int to string) and throw a `ProcessingException` withouth the error type prefix
--
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 #11440: Return better error info when no servers instances can be found in mu…
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11440:
URL: https://github.com/apache/pinot/pull/11440#discussion_r1305429967
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerManager.java:
##########
@@ -228,8 +234,13 @@ private void assignWorkersToPartitionedLeafFragment(DispatchablePlanMetadata met
tableName, i);
ServerInstance serverInstance =
pickEnabledServer(partitionInfo._fullyReplicatedServers, enabledServerInstanceMap, indexToPick++);
- Preconditions.checkState(serverInstance != null,
- "Failed to find enabled fully replicated server for table: %s, partition: %s in table: %s", tableName, i);
+ if (serverInstance == null) {
Review Comment:
are we able to catch this exception all the way at top? If so, we don't have to add the these many `throws ProcessingException`
--
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 #11440: Return better error info when no servers instances can be found in mu…
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11440:
URL: https://github.com/apache/pinot/pull/11440#issuecomment-1693123305
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11440?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11440](https://app.codecov.io/gh/apache/pinot/pull/11440?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (d34467d) into [master](https://app.codecov.io/gh/apache/pinot/commit/d91e5db76c53af9774db1c57c71520f4c196c059?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (d91e5db) will **decrease** coverage by `0.13%`.
> The diff coverage is `25.92%`.
```diff
@@ Coverage Diff @@
## master #11440 +/- ##
============================================
- Coverage 62.98% 62.85% -0.13%
+ Complexity 1107 1105 -2
============================================
Files 2303 2303
Lines 124041 124062 +21
Branches 18895 18896 +1
============================================
- Hits 78133 77985 -148
- Misses 40353 40510 +157
- Partials 5555 5567 +12
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11440/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/11440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/11440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/11440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/11440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.47% <11.11%> (-48.50%)` | :arrow_down: |
| [java-17](https://app.codecov.io/gh/apache/pinot/pull/11440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.85% <25.92%> (+<0.01%)` | :arrow_up: |
| [java-20](https://app.codecov.io/gh/apache/pinot/pull/11440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.47% <11.11%> (-48.38%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.85% <25.92%> (-0.13%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.85% <25.92%> (-0.13%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `67.31% <20.00%> (-0.21%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/11440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.49% <11.11%> (+0.01%)` | :arrow_up: |
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/11440?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...t/query/planner/physical/PinotDispatchPlanner.java](https://app.codecov.io/gh/apache/pinot/pull/11440?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcGxhbm5lci9waHlzaWNhbC9QaW5vdERpc3BhdGNoUGxhbm5lci5qYXZh) | `83.33% <ø> (ø)` | |
| [.../org/apache/pinot/query/routing/WorkerManager.java](https://app.codecov.io/gh/apache/pinot/pull/11440?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcm91dGluZy9Xb3JrZXJNYW5hZ2VyLmphdmE=) | `67.53% <0.00%> (-1.29%)` | :arrow_down: |
| [.../java/org/apache/pinot/query/QueryEnvironment.java](https://app.codecov.io/gh/apache/pinot/pull/11440?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvUXVlcnlFbnZpcm9ubWVudC5qYXZh) | `87.60% <20.00%> (-3.00%)` | :arrow_down: |
| [.../apache/pinot/common/exception/QueryException.java](https://app.codecov.io/gh/apache/pinot/pull/11440?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `88.11% <33.33%> (-5.43%)` | :arrow_down: |
| [...requesthandler/MultiStageBrokerRequestHandler.java](https://app.codecov.io/gh/apache/pinot/pull/11440?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvTXVsdGlTdGFnZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `23.30% <42.85%> (-0.73%)` | :arrow_down: |
... and [26 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11440/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] gortiz commented on a diff in pull request #11440: Return better error info when no servers instances can be found in mu…
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #11440:
URL: https://github.com/apache/pinot/pull/11440#discussion_r1305477016
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerManager.java:
##########
@@ -228,8 +234,13 @@ private void assignWorkersToPartitionedLeafFragment(DispatchablePlanMetadata met
tableName, i);
ServerInstance serverInstance =
pickEnabledServer(partitionInfo._fullyReplicatedServers, enabledServerInstanceMap, indexToPick++);
- Preconditions.checkState(serverInstance != null,
- "Failed to find enabled fully replicated server for table: %s, partition: %s in table: %s", tableName, i);
+ if (serverInstance == null) {
Review Comment:
We are catching the exception in `MultiStageBrokerRequestHandler`, but given it is a checked exception (doesn't extend Error or RuntimeException) it is mandatory to add the exception in the throws section in order to compile that in Java. Is there something I'm not understanding?
In future, if we decide to add a new exception decoupled from the wire protocol (and therefore it doesn't extends `org.apache.thrift.TException`) we can extend RuntimeException instead of Exception in order to do not need to add these throws.
I don't have a clear preference between checked exceptions, unchecked exceptions and a more functional way (where errors are returned as arguments like Status<R> where Status is a sealed class that can either contain the actual result or an exception). For example Kotlin only supports the latest two patterns and it felt very natural to me, but I also find useful to have checked exceptions sometimes.
--
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] gortiz commented on a diff in pull request #11440: Return better error info when no servers instances can be found in mu…
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #11440:
URL: https://github.com/apache/pinot/pull/11440#discussion_r1305477016
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerManager.java:
##########
@@ -228,8 +234,13 @@ private void assignWorkersToPartitionedLeafFragment(DispatchablePlanMetadata met
tableName, i);
ServerInstance serverInstance =
pickEnabledServer(partitionInfo._fullyReplicatedServers, enabledServerInstanceMap, indexToPick++);
- Preconditions.checkState(serverInstance != null,
- "Failed to find enabled fully replicated server for table: %s, partition: %s in table: %s", tableName, i);
+ if (serverInstance == null) {
Review Comment:
We are catching the exception in `MultiStageBrokerRequestHandler`, but given it is a checked exception (doesn't extend Error or RuntimeException) it is mandatory to add the exception in the throws section in order to compile that in Java. Is there something I'm not understanding?
In future, if we decide to add a new exception decoupled from the wire protocol (and therefore it doesn't extends `org.apache.thrift.TException`) we can extend RuntimeException instead of Exception in order to do not need to add these throws.
I don't have a clear preference between checked exceptions, unchecked exceptions and a more functional way (where errors are returned as arguments like Status<R> where Status is a sealed class that can either contain the actual result or an exception). For example Kotlin only supports the latest two patterns and it felt very natural to me.
--
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] gortiz commented on a diff in pull request #11440: Return better error info when no servers instances can be found in mu…
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #11440:
URL: https://github.com/apache/pinot/pull/11440#discussion_r1305413347
##########
pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java:
##########
@@ -180,6 +185,20 @@ public static ProcessingException getException(ProcessingException processingExc
return copiedProcessingException;
}
+ public static void decorateException(ProcessingException processingException) {
+ String errorType;
+ switch (processingException.getErrorCode()) {
+ case SERVER_INSTANCE_MISSING_ERROR_CODE:
+ errorType = "ServerInstanceMissingError";
+ break;
+ // TODO: add more branches
Review Comment:
I don't like this idea (used above in `getException`) where we have a base exception with the message and then we do a deep copy. IMHO it would be better to just have an exception class for each ProcessingException or a builder method that takes the error code and the message and does something like this method, creating the exception with the `errorType` prefix associated with the given error code.
I'm not using this method as I would like because I'm following the existent pattern where the exception is first thrown without the `errorType` prefix and then it is decorated.
--
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 #11440: Return better error info when no servers instances can be found in mu…
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11440:
URL: https://github.com/apache/pinot/pull/11440#issuecomment-1699709201
> @Jackie-Jiang I don't understand how do you propose to make the message more crisp. How do you know, in pinot-`MultiStageBrokerRequestHandler` (or `QueryEnvironment`) the actual error reason? The only way I think we have is to parse the message of the exception (looking for something that starts with `Failed to find enabled fully replicated server for table:`), which seems like a very bad idea. Is there something I'm missing?
>
> Remember that we don't just want to change the error message, we also want to provide a more specific error code. In my example we return 715 instead of the generic 150.
>
> Do you propose to include the exception trace or not?
We can include the exception trace because that is very useful debugging info when user run into issues. For this specific error, the most confusing part is the error type because the actual exception has nothing to do with query parsing (the query syntax is completely legit). IMO separating query parsing error and planning error is good enough for now.
Basically I'm okay if it returns something like:
```
[
{
"message": "QueryPlanningError:\nError planning query plan\norg.apache.pinot.broker.requesthandler.MultiStageBrokerRequestHandler.handleRequest(MultiStageBrokerRequestHandler.java:137)\norg.apache.pinot.broker.requesthandler.BaseBrokerRequestHandler.handleRequest(BaseBrokerRequestHandler.java:263)\norg.apache.pinot.broker.requesthandler.BrokerRequestHandler.handleRequest(BrokerRequestHandler.java:48)\nFailed to find enabled fully replicated server for table: <tableName>, partition: <i> in table: %s\nai.startree.shaded.com.google.common.base.Preconditions.checkState(Preconditions.java:802)\norg.apache.pinot.query.routing.WorkerManager.assignWorkersToPartitionedLeafFragment(WorkerManager.java:231)\norg.apache.pinot.query.routing.WorkerManager.assignWorkersToLeafFragment(WorkerManager.java:119)\norg.apache.pinot.query.routing.WorkerManager.assignWorkersToNonRootFragment(WorkerManager.java:85)\n",
"errorCode": 150
}
]
```
--
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 pull request #11440: Return better error info when no servers instances can be found in mu…
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #11440:
URL: https://github.com/apache/pinot/pull/11440#issuecomment-1707589340
There is one more issue in `MultiStageEngineIntegrationTest`.
--
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