Repository navigation
feat(storage): multi-stream support in AsyncMultiRangeDownloader - #18537
raj-prince wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces multi-stream scaling and unfinalized object support in AsyncMultiRangeDownloader (MRD) by implementing an internal stream pool (_StreamPool) and stream manager (_ManagedStream). Feedback on these changes focuses on optimizing concurrency and resource management. Key recommendations include releasing the lock before awaiting the stream factory in acquire_stream to prevent blocking concurrent tasks, tracking background tasks to avoid garbage collection, initializing and updating self._routing_token to prevent AttributeError and preserve routing state, and properly handling pool closure to avoid resource leaks.
1e84d23 to
82444d4
Compare
|
/gcbrun(b183f7f) |
|
some comments on descriptions
Just mention single-stream (because each stream maps to 1 grpc channel , the underlying TCP connections per channel depends on the gRPC's xDS for direct path)
change this to single-stream
Aren't we going with the hybrid approach ? |
|
/gcbrun |
| if len(tasks) == 1: | ||
| await tasks[0] |
There was a problem hiding this comment.
these 2 lines are not necessary.
There was a problem hiding this comment.
Special handling was for performance purpose where all ranges will be in a single task. Added comment.
| if current_stream is None or current_stream.is_fully_loaded( | ||
| self.target_io_depth, self.target_bytes | ||
| ): | ||
| current_stream = self._select_stream() |
There was a problem hiding this comment.
should we use acquire_stream instead ?
There was a problem hiding this comment.
acquire_stream is not required not given distribute_ranges does the work of allocating stream for a given range. Renamed the _select_stream to _acquire_stream and removed the original acquire_stream (with lock).
Description
This PR adds optional multi-stream support to
AsyncMultiRangeDownloader(MRD), enabling downstream consumers likegcsfsto scale throughput beyond single-stream limits on high-bandwidth storage classes like RAPID.By default, existing single-stream behavior is fully preserved with zero pooling or load-tracking overhead (no lock acquisition or pending range/byte accounting). Multi-stream is strictly opt-in via the
stream_configparameter.Summary of changes:
MRDStreamConfig: Added a configuration dataclass to bundle multi-stream settings (min_streams,max_streams,target_io_depth,target_bytes).stream_config=Noneby default): Whenstream_configis omitted, the pool is completely bypassed (_pool = None). Requests execute directly on the primary stream with no byte/range counting, locking, or load calculation._StreamPool): When enabled, range requests are distributed across streams using a hybrid selection strategy (least-loaded across basemin_streams, first-fit across dynamically scaled streams). If load exceeds target capacity, additional streams are opened in the background in proportion to overall load without blocking active reads on already-open streams.read_handleandrouting_token(with redirect-aware retry handling) to connect directly to the same backend without repeating lookup overhead.Benchmark Results (20 GiB download on RAPID)
min_streams = max_streams)test_reads.py execution
All 6 benchmark cases in test_reads.py executed and PASSED (total elapsed time:
3m 49.37s):seq_regional_96p_1c_10240MiB_64KiB_1rangesseq_rapid_96p_1c_10240MiB_64KiB_1rangesrand_regional_1p_1c_10240MiB_64KiB_1rangesrand_regional_1p_16c_10240MiB_64KiB_1rangesrand_rapid_1p_1c_10240MiB_64KiB_1rangesrand_rapid_1p_16c_10240MiB_64KiB_1ranges