Improve exceptions broker #14994
Improve exceptions broker #14994
Conversation
Introduce a new PinotRuntimeException and modify other SPI exceptions to extend from it.
gortiz
force-pushed
the
improve_exceptions_broker
branch
from
b9c2b52 to
6729d27
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14994 +/- ##
=============================================
- Coverage 61.75% 33.94% -27.82%
- Complexity 207 680 +473
=============================================
Files 2436 2718 +282
Lines 133233 152591 +19358
Branches 20636 23541 +2905
=============================================
- Hits 82274 51792 -30482
- Misses 44911 96615 +51704
+ Partials 6048 4184 -1864
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| * exception, this exception is designed to be thrown and caught by the query engine as any other standard Java | ||
| * exception. | ||
| */ | ||
| public class QException extends PinotRuntimeException { |
There was a problem hiding this comment.
Suggest naming this QueryException. Since you are cleaning up org.apache.pinot.common.exception.QueryException, there is a TODO there to rename that to QueryExceptionUtils, and you can do it together to avoid name conflict
There was a problem hiding this comment.
I didn't want to have an even longer PR, but if you think it is the best way, I'll try to do that as well
| import java.util.Map; | ||
|
|
||
|
|
||
| public class MdcRequestScope implements RequestScope { |
There was a problem hiding this comment.
Please add some javadoc. What does mdc stand for?
| public float toFloat(Object value) { | ||
| // NOTE: No need to trim here because Float.valueOf() will trim the string | ||
| return Float.parseFloat(value.toString()); | ||
| try { |
There was a problem hiding this comment.
This class is used in a lot of places and we want to ensure highest performance. It expects the input to be properly formatted, and it is not only used by the query engine. Can you do the try-catch on the caller side?
| public void shutDown() { | ||
| _queryDispatcher.shutdown(); | ||
| } | ||
|
|
|
I'm convering the PR into a draft given I've found some stack traces leaking in the tests. My plan is to apply some extra changes. If it ends up being too difficult to review I'll try to split it once I reach the desired state |
|
This have been split into several other PRs. Some of them have been merged and #15277 is at the time the only one open |
3 participants
This extensive PR is focused on improving error handling on MSE, but partially in SSE and TSE. Error handling includes changes in the error responses sent to clients but also logs in servers, brokers and controllers.
Logging has been reduced to not logging stack traces when they are not useful. Logging is also improved to include the request-id in MSE and SSE queries and stage id in MSE queries. Instead of spreading the query context to all places where logs are created, this PR injects these values using MDC. This means that:
This PR tries to solve most problems in #14950. I was trying to split changes in different PRs, but given how each change affects the other, it was difficult.
It is difficult to compare logs in here (or to add tests that verify logs), so I've created this document showing the changes. Notice there is one tab per case of study