feat: optimize partial loads by coalescing (or splitting) range reads#19683
Conversation
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
Reviewed 29 of 29 changed files.
This is an automated review by Codex GPT-5.6-Sol
| // callback inline on THIS thread — but the callback deserializes columns and can even hit deep storage | ||
| // (projection parent columns load lazily), which must not run on a processing thread. Submit one token task | ||
| // so the callback hops to the download executor like every other build. | ||
| runDownloads.add(bundleAcquirer.submitDownload(() -> "resident")); |
There was a problem hiding this comment.
[P1] Keep holder construction off the caller thread
The no-op token can complete before AsyncResources.collect and addReadyCallback are wired, and callbacks registered on an already-ready resource execute synchronously. In that ordering, the callback materializes columns and may lazily download projection-parent files on the processing thread, violating the async I/O contract. Schedule the continuation itself on the download executor instead of relying on the token's completion thread.
| for (String name : runFiles) { | ||
| final ReentrantLock lock = fileLocks.computeIfAbsent(name, k -> new ReentrantLock()); | ||
| locks.add(lock); | ||
| lock.lock(); |
Copying from the prior PR |
yes, though sort of indirectly, there is a set of locks associated with each internal file of the segment which dedupe calls to the same range, so one caller does the work and the others just wait and pick up the result. These locks are acquired in a stable order so that competing callers dont lock each other out. The locking isn't new in this PR, the changes here are more around efficiency/optimization.
this is tunable, |
Description
This PR optimizes partial loads by coalescing range reads into a single request when the gap between them is under a threshold (default 1MiB) as well as split range reads into multiple requests when over a threshold (default 64 MiB) to attempt to achieve a balance of maximizing parallelism, but sensibly so that we don't suffer too much overhead from having too many individual requests.
new configs:
druid.storage.virtualStorageCoalesceGapBytesdruid.storage.virtualStorageMaxFetchRunBytesThis transitions the prior load by side-effect behavior of partial loads into something planned and purposeful; to support this V10 segments now have some additional metadata which tracks which column internal files are associated with, making it easy to translate a
CursorBuildSpecinto an explicit list of the internal files needed to serve it. The prior load-by-side-effect behavior should not happen in any production paths, as this PR transitions any v10 segments written prior to this metadata change to fall-back to 'download the entire bundle' behavior.This change also fixes a bug with partial load of nested columns, which prior to this change was missing the loading of nested field columns, so any query using them would incorrectly perform downloads on the processing thread (sad). This is fixed because now all of those internal files are associated with the nested column, so we can fetch the entire set when we intend to. This is actually not ideal in most cases, I have planned some future work to make improvements to the
CursorBuildSpec-> internal files list to recognize nested field virtual columns and do a more targeted download to only fetch the internal files associated with the field instead of all fields belonging to the json column (but not in this PR).