feat(storage): add multistream support to MRD

[krishnamd-jkp]

No description provided.

[gemini-code-assist]

Summary of Changes

Hello @krishnamd-jkp, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly upgrades the MultiRangeDownloader (MRD) by integrating multistream capabilities. This enhancement allows the MRD to leverage multiple concurrent gRPC streams for downloading object ranges, which can lead to improved throughput and reduced latency, especially for large objects or high-concurrency scenarios. The changes include mechanisms for dynamic stream creation, intelligent load distribution of range requests across these streams, and new configuration options to fine-tune the multistream behavior based on application requirements.

Highlights

• Multistream Support for MRD: The MultiRangeDownloader (MRD) now supports using multiple concurrent bidirectional gRPC streams to download object ranges, enhancing download efficiency.
• Dynamic Stream Management: The system can dynamically create and manage multiple streams, adding new streams when existing ones reach configurable pending range or byte thresholds, up to a maximum connection limit.
• Configurable Stream Behavior: New options (minConnections, maxConnections, targetPendingRanges, targetPendingBytes) are introduced to allow users to control the number of streams and the conditions under which new streams are added.
• Load Balancing: A minBytesPicker strategy is implemented to distribute range requests among active streams, aiming to balance the outstanding bytes across connections.

Changelog

• storage/client.go
  • Added minConnections, maxConnections, targetPendingRanges, and targetPendingBytes fields to newMultiRangeDownloaderParams for configuring multistream behavior.
• storage/grpc_reader_multi_range.go
  • Introduced new constants: mrdSendChannelSize, defaultTargetPendingBytes, defaultTargetPendingRanges.
  • Defined streamPickerStrategy interface and minBytesPicker implementation for selecting streams for range requests.
  • Added mrdStream struct to manage individual stream state, including pending ranges and byte counts.
  • Refactored multiRangeDownloaderManager to maintain a map of mrdStream instances, replacing the single currentSession.
  • Implemented shouldAddStream and addNewStream functions to dynamically create new streams based on connection limits and pending load thresholds.
  • Updated the eventLoop to handle commands and session results from multiple streams, and to trigger new stream creation.
  • Modified createNewSession to accept a stream id and to optionally wait for the first result.
  • Adjusted handleAddCmd to use the streamPicker for routing range requests to specific streams and updating stream-specific metrics.
  • Updated processSessionResult, handleStreamEnd, ensureSession, failRange, and failAllPending to correctly manage state and operations across multiple streams.
  • Introduced addStreamCmd and mrdAddStreamErrorCmd for internal stream management.
  • Modified mrdSessionResult to include the stream id.
  • Changed bidiReadStreamSession to store its id.
  • Introduced mrdQueuedRequest to associate queued requests with their target stream id.
• storage/reader.go
  • Defined mrdOption interface and concrete option types (minConnections, maxConnections, targetPendingRanges, targetPendingBytes) for configuring MultiRangeDownloader.
  • Updated NewMultiRangeDownloader to accept and apply these new mrdOptions.

Activity

• No specific activity (comments, reviews, etc.) was provided in the context. The pull request was created by krishnamd-jkp.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces multistream support for the Multi-Range Downloader (MRD), refactoring the single-stream architecture to manage multiple concurrent download streams. However, it introduces a critical race condition that can lead to a nil pointer dereference and a medium-severity data race. This occurs because a new stream can send responses to the manager before it's fully tracked, and the gRPC read specification is shared and modified across multiple goroutines without proper synchronization. Additionally, there is a suggestion regarding the public API for configuring the new multistream options to improve consistency.

[krishnamd-jkp]

/gemini review

[gemini-code-assist]

Code Review

This pull request adds support for multiple concurrent streams to the Multi-Range Downloader (MRD). However, a critical data race was identified in the addNewStream method, which needs to be addressed for reliability. Additionally, a significant resilience issue exists where a single stream failure can bring down the entire downloader, and clarifications to the documentation for new configuration options are suggested.

[cpriti-os]

Initial review

[cpriti-os]

This is a good addition and makes tests more reliable but also adds operation consuming CPU in production with no real functionality, can we do something so that this stats tracking is disabled in actual production? On the other hand, I do believe these are good stats and we might be interested in it for advanced observability.

[cpriti-os]

/gemini review

[gemini-code-assist]

Code Review

This pull request adds multistream support to the Multi-Range Downloader (MRD), which is a significant feature enhancement. The changes are extensive and well-structured, introducing concepts like stream pickers, dynamic connection scaling, and robust session management. The core logic appears sound. I've identified a critical issue regarding the usability of the new options and a minor bug in one of the new integration tests. Once these are addressed, this will be a great addition.