Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces range coalescing for chunk payload reads by integrating with zarrita's withRangeCoalescing utility. This allows concurrent reads from the same backing object to be merged into fewer, larger requests, improving efficiency for both native and virtual chunks. The implementation includes a new range-coalescer.ts module, an LRU cache for coalesced stores in ReadSession, and updates to the IcechunkStore API to expose the rangeCoalescing option. Feedback is provided regarding the restoration of range details in error messages for native chunk fetch failures to maintain observability.
| throw new Error( | ||
| `Storage returned ${data.length} bytes for ${path} range ${range.start}-${range.end - 1}; expected ${payload.length} bytes`, | ||
| `Storage returned ${data.length} bytes for ${path}; expected ${payload.length} bytes`, | ||
| ); |
There was a problem hiding this comment.
The error message for native chunk fetch failures has been simplified, removing the specific range information. While cleaner, this might make debugging harder if a storage backend returns an unexpected length. Consider keeping the range details in the error message for better observability.
| throw new Error( | |
| `Storage returned ${data.length} bytes for ${path} range ${range.start}-${range.end - 1}; expected ${payload.length} bytes`, | |
| `Storage returned ${data.length} bytes for ${path}; expected ${payload.length} bytes`, | |
| ); | |
| throw new Error( | |
| `Storage returned ${data.length} bytes for ${path} range ${payload.offset}-${payload.offset + payload.length - 1}; expected ${payload.length} bytes`, | |
| ); |
Adds opt-in range coalescing for icechunk chunk payload reads.
src/reader/range-coalescer.tsadapters expose icechunk's two read paths as zarritaAsyncReadable:makeStorageStore— native chunks viaStoragemakeUrlStore— virtual chunks via direct HTTP, with optional conditional headers forvalidateChecksumsReadSessionkeeps a bounded LRU of coalescer-wrapped stores, partitioned by URL, fetch client, and checksum identity so conditional headers and per-client state never bleed across a coalesced batch.AsyncReadableshape locally and only run user-suppliedwithRangeCoalescing.Range(HTTP 200 fallback) by slicing the requested window out of the full body, plus suffix-length range support.