Fix pinot connector ignoring certain exceptions silently#25424
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
rhodo
changed the title
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
| String responseType = response.getMetadataMap().get(MetadataKeys.RESPONSE_TYPE); | ||
| if (responseType.equals(ResponseType.METADATA)) { | ||
| if (responseType.equals(ResponseType.METADATA) | ||
| && !PinotSessionProperties.isGrpcQueryEnforceMetadataException(session)) { |
There was a problem hiding this comment.
The idea is that instead of using metadata type to signal the end of data, we rely on response iterator check here to signal the end of data
| private boolean countDistinctPushdownEnabled = true; | ||
| private boolean proxyEnabled; | ||
| private DataSize targetSegmentPageSize = DataSize.of(1, MEGABYTE); | ||
| private boolean grpcQueryEnforceMetadataException; |
There was a problem hiding this comment.
I'm not a fan of making exception handling configurable based on a configuration.
@elonazoulay Do you have any other idea?
There was a problem hiding this comment.
I agree, especially since it's a choice between empty results masking an exception or a query failure with detailed exception (so it can be investigated and resolved).
Is there any issue with always surfacing exceptions when the query fails?
There was a problem hiding this comment.
Thanks for reviewing! One reason we were considering making this behavior configurable is that, today, there are cases where partial results are returned instead of raising an exception—for example, when one or a few segments are missing for a table. Some users may be fine with receiving most of the query's result, while others, especially those with stricter consistency requirements, might prefer to get an explicit segment-missing exception.
That said, if we can align on always surfacing exceptions, it would certainly simplify the logic.
(cc @xiangfu0)
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
addressed comments, thanks for reviewing @ebyhr ! |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
Can we please get this PR reviewed? |
elonazoulay
left a comment
elonazoulay
left a comment
There was a problem hiding this comment.
lgtm overall % exception comment!
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
5 participants
Description
When making grpc requests towards pinot server, certain exceptions are carried back in response of metadata type, for example, query timeout exception, segment missing exception etc. Due to the fact that EndOfData() return too early pinot connector actually never has a chance to deserialize these exceptions, which means it may return empty or partial result instead of raising an exception. Fixing this by still allow payload of metadata response being inspected, and to optionally maintain backward compatibility, a config/session property knob is introduced to gate the new behavior.
To test this, I spinned up a pinot cluster with extremely small timeout
pinot.server.query.executor.timeout=1and let trino connect to it.Before the fix, trino will silently return the empty result even underlying it hits timeout exception
with the
pinot.grpc.query.enforce-metadata-exception=trueset in pinot.properties in the pinot connector, timeout exception is raised to end userAdditional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: