-
Notifications
You must be signed in to change notification settings - Fork 50
Exception detection respect tag #514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
peter-leonov-ch
wants to merge
18
commits into
main
Choose a base branch
from
exception_detection_respect_tag
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
de2e8ad to
26f1c31
Compare
d6e84b5 to
5998181
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
26f1c31 to
16c55a7
Compare
6543958 to
6d4e067
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
The current exception detection code is implemented in a way that makes any combination of
\r\nthrow an exception. This works fine for most of the use cases of the JS client as the JSON-based family of response formats is the most widely used and JSON guarantees that the output stream is not going to have a non-escaped\rbyte.This heuristics does not work for
\r\nterminated CSV. A CH client created with the following configuration will trip on the legitimate output:The JS exception in this scenario describes that there was an exception on the server side but parsing of that exception did not go well. This might be confusing.
What
This PR proposes to use a more strict error parser for the supported raw formats (based on the list in
SupportedRawFormats). This stricter parser is less likely to trip on valid input.The downside is that neither of the parsers are byte-level (meaning they require a certain amount of the stream to be buffered). This introduces a higher risk of the exception marker and message not being recognized (or only partially recovered) when the exception byte sequence is split by a (network, in our case HTTP) chunk boundary. For example:
My take on this is that for raw formats it seems to be a better trade-off to potentially miss an exception compared to not being able to process output at all.
This PR also fixes an infinite loop case scenario that happens on a certain exception message split.
Also
The PR uses blanket implementation for all the raw formats not taking the
\rsetting into account. It might be better to introduce a separate client config option instead that default to the simpler JSON-friendly parser. This may help get consistent results for those rare cases with\rterminated SCV and custom output format.Also, one of the added tests has to have a certain number of output rows to run in both
localandlocal_clustermodes. This is because inlocal_clustermodenginxrepacks the HTTP chunks using it's internal HTTP acceleration-focused logic and changes this way the resulting chunking conditions as seen by the client.Checklist
Delete items not relevant to your PR: