fix: reverting default cat_file concurrency back to 1 - #1051
Conversation
There was a problem hiding this comment.
Code Review
This pull request decouples the default concurrency settings between cat_file and the prefetcher, reverting DEFAULT_CONCURRENCY to 1 while introducing DEFAULT_PREFETCHER_CONCURRENCY with a default of 4. Feedback includes validating that the DEFAULT_GCSFS_CONCURRENCY environment variable is strictly positive to avoid division-by-zero or other runtime errors, and removing duplicated text in the changelog entry.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1051 +/- ##
=======================================
Coverage 90.18% 90.19%
=======================================
Files 16 16
Lines 3740 3742 +2
=======================================
+ Hits 3373 3375 +2
Misses 367 367 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks. By itself I think this is (also) a good solution for the problem we are observing. I tried to find out whether this unnecessarily leaves performance gains behind.. zarr always uses But is that a problem? Do other libraries also use this path? I had Claude do a bit of analysis and put the gist here: https://gist.github.com/dhrp/c8804ae2ac0a2e16961366d0107d5499 |
0a68fd7 to
c3743e6
Compare
c3743e6 to
2acdf11
Compare
Thank again for the analysis, discussed with the team and decided to go ahead with minimal safer change:
|
2acdf11 to
e0b743f
Compare
|
Just saw this in your description, can you talk more on this?
I just tested reading both 0-byte objects and directory placeholder objects (0 bytes) on 2026.8.0, and both scenarios cleanly returned empty bytes (b'') without throwing a 416 error. I thought the only concern is the additional HTTP call. |
My bad, that's AI generated summary - fixed it. Yes, the only concern is the additional HTTP call. |
|
You would need a change here as well |
|
Sounds good! I'll close my PR |
Problem
In release 2026.8.0,
cat_filestarted usingconcurrency=DEFAULT_CONCURRENCY(4). Becauseconcurrency > 1by default, allcat_filecalls branched into_cat_file_concurrent. When reading files of unknown size—such as Zarr metadata, Parquet metadata, and small files—this triggered an upfront_info()call, issued speculative HTTP range requests with extra round-trip latency. (#1048).Solution
We restore single-request sequential reads as the default for
cat_fileby acceptingconcurrencyviakwargsand defaulting to1:cat_filedefaults to sequential (concurrency=1): Restores standard single-requestGET(200 OK) without extra round-trips,_info()lookups.concurrency=...viakwargs(e.g.,fs.cat_file(path, concurrency=4)) when concurrent fetching of large objects is desired.GCSFile,ZonalFile, and_get_filecontinue to default toDEFAULT_CONCURRENCY = 4for high-throughput streaming reads.Microbenchmark cat_file result
1. Without Concurrency (
concurrency=1)2. With Concurrency (
concurrency=4)Closes #1048