Repository navigation
perf(thrift): buffer reply body, use accelerated protocol, log payloads lazily - #1020
vuanhphung wants to merge 7 commits into
Conversation
…ds lazily THttpClient.read() forwarded every primitive read of the pure-Python thrift decoder to urllib3, which dominated decode time for wide result schemas and column-based metadata results. Read the reply body once, expose it as a CReadableTransport, and decode with TBinaryProtocolAccelerated (falls back to pure Python when thrift's C extension is unavailable). unsafe_logger.debug() pre-formatted every request and response with str.format, stringifying whole payloads (including inline Arrow batches) even though the logger only has a NullHandler by default. Pass them as %s args. Signed-off-by: Vu Anh Phung <vu.phung@databricks.com> Co-authored-by: Isaac <no-reply@databricks.com>
GitHub Actions
Error
GitHub Actions
Inactive
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Looks good overall: the buffered CReadableTransport and the lazy %s logging are both correct, and the existing string/container limits are kept. One medium concern: the buffered reply body stays referenced after decoding, which roughly doubles memory held per connection for large inline results. There is also a minor inconsistency in the exception raised for a truncated reply.
Locks in that the transport's reply buffer is cleared once a successful RPC returns (make_request closes the transport in its finally block). Signed-off-by: Vu Anh Phung <vu.phung@databricks.com> Co-authored-by: Isaac <no-reply@databricks.com>
GitHub Actions
Error
GitHub Actions
Inactive
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com> Co-authored-by: Isaac <no-reply@databricks.com>
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
There was a problem hiding this comment.
Verdict: 2 Low
Looks good. Buffering the body and using the accelerated protocol are both correct, and the transport is closed in make_request's finally, which releases the buffer. There are two low-severity notes: peak memory now roughly doubles for large inline replies, and the "C path" test can pass without the C extension ever running.
TBinaryProtocolAccelerated silently falls back to pure Python when thrift's C extension isn't built, so add a test (skipped without fastbinary) that asserts the C decoder actually ran on the buffered body. Share reply/request builders across the transport tests, and note in the transport that the raw reply is held alongside the decoded objects until close(). Signed-off-by: Vu Anh Phung <vu.phung@databricks.com> Co-authored-by: Isaac <no-reply@databricks.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
There was a problem hiding this comment.
Verdict: 2 Low
Looks good overall, with two low-severity concerns. The buffered transport correctly implements CReadableTransport, close() resets the read buffer, error-handling paths in make_request are unchanged, and the lazy %s logging keeps the same output. The two concerns are the untested switch to C-side request encoding and a comment that overstates how the buffered body's size is bounded.
…fer bound TBinaryProtocolAccelerated also moves request encoding to C. Add a test (skipped without fastbinary) that builds OpenSession, ExecuteStatement (with native parameters and query tags), GetTables and GetColumns requests through the backend and checks both encoders produce identical bytes. Clarify that the buffered reply is bounded by maxBytes only for fetches. Signed-off-by: Vu Anh Phung <vu.phung@databricks.com> Co-authored-by: Isaac <no-reply@databricks.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Failure
There was a problem hiding this comment.
Verdict: 1 Low
Looks good. The buffered CReadableTransport and the switch to TBinaryProtocolAccelerated look correct: the MRO is fine, truncation raises EOFError the same way the pure-Python path does, read errors still surface inside make_request's existing urllib3/OSError handlers, and close() in finally releases the buffer. The lazy %s logging is safe because the only args-mutating filter is attached to urllib3. One low note about the peak-memory trade-off.
GitHub Actions
Error
GitHub Actions
Inactive
…whole Reading the whole body in flush() held a second copy of it during decode and, on urllib3 1.26, turned a truncated 503/429 body into a ProtocolError that skipped the status-based retry. The transport now refills from urllib3 in 1 MiB chunks (or a whole field if larger), like TBufferedTransport, so truncated bodies fail inside decode as on main. Peak heap for a 100 MiB multi-batch reply drops from ~200 to ~103 MiB at the same speed. Tests now run the decode paths under both the C and the pure-Python protocol, and cover the truncated-503 retry. Signed-off-by: Vu Anh Phung <vu.phung@databricks.com> Co-authored-by: Isaac <no-reply@databricks.com>
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Verdict: 1 Low
Looks good, with one low concern. The chunked CReadableTransport matches what TBufferedTransport does, the read buffer is reset on flush and close, and the new tests cover both protocol paths, a truncated 503 that still gets retried, and the lazy logging. The one low concern is that cstringio_refill assembles large fields with bytes +=, which copies quadratically when urllib3 returns short reads.
If urllib3 returns short reads for a field larger than a chunk, bytes += recopied everything read so far on every read. Collect the pieces and join them once. Adds a test that decodes a reply from short reads on both protocols. Signed-off-by: Vu Anh Phung <vu.phung@databricks.com> Co-authored-by: Isaac <no-reply@databricks.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Inactive
What type of PR is this?
Description
Two client-side costs on the Thrift backend showed up in the nightly driver benchmark:
THttpClient.read()forwarded every primitive read of the pure-Python Thrift decoder to urllib3. For replies with many fields (wide result schemas, column-based metadata results) that per-call overhead dominated: decoding a 2,415-rowgetColumnsreply took ~370 ms vs ~1.3 ms from a buffered body with the C decoder. The transport now reads the body from urllib3 in 1 MiB chunks (or a whole field if larger), likeTBufferedTransport, and implementsCReadableTransport. The backend usesTBinaryProtocolAccelerated, which falls back to pure Python when Thrift's C extension isn't built. Because the body is never read whole, a 503/429 with a truncated body still reaches the status-based retry on urllib3 1.26, and the extra memory during decode is about one chunk or one field rather than the whole reply. String/container limits are unchanged; >15.6 MiB fields still decode on thrift 0.24 and 0.25.unsafe_logger.debug("...".format(payload))stringified every request and response even though that logger only has aNullHandlerby default (~25 ms per MB of inline Arrow). The payloads are now passed as%sargs, so output is unchanged when a handler is attached.Live A/B against a Medium PRO warehouse (p50 of 3 interleaved rounds, main → this PR):
tables()across all catalogs (60k rows)columns()onsamples(2.5k rows)LIMIT 0query, client time minus server timeSELECT 1How is this tested?
New unit tests cover the chunked transport under both the C and the pure-Python protocol, a full RPC decoded through the C path, the truncated-503 retry, and the lazy log calls. The unit suite passes on thrift 0.24 and 0.25, and with the C extension disabled. The Thrift e2e tests in
test_driver.py,test_complex_types.py,test_variant_types.pyandtest_parameterized_queries.pypass against a live warehouse. I didn't run the retry or long-running suites, or the UC volume and streaming-PUT tests, which need a volume fixture.Related Tickets & Documents
N/A
This pull request and its description were written by Isaac.