feat: add pending request channel size configuration to ConnectionConfig #1328
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
semver-checks-breaking
|
See the following report for details: cargo semver-checks output |
|
@dhvll CI is failing. |
It's not that important feature, but cpp-rust-driver requires it. See the issue: #1313 |
|
I'll look into it. |
Lorak-mmk
requested changes
May 2, 2025
Comment on lines
+334
to
+357
|
|
||
| /// Set the size of the pending request channel for each connection. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `size` - The maximum number of pending requests per connection. | ||
| /// | ||
| /// # Notes | ||
| /// | ||
| /// - This is different from cpp-driver's implementation, which uses a per-RequestProcessor queue. | ||
| /// - The default is 2048, a balanced value between performance and memory usage. | ||
| /// - Adjust based on your specific workload and system resources. | ||
| /// | ||
| /// # Example | ||
| /// | ||
| /// ``` | ||
| /// let session = SessionBuilder::new() | ||
| /// .connection_config( | ||
| /// ConnectionConfig::new() | ||
| /// .with_pending_request_channel_size(4096) | ||
| /// ) | ||
| /// .build() | ||
| /// .await?; | ||
| /// ``` |
There was a problem hiding this comment.
I think it is worth describing what a "pending request" is. I think users may think that it is a request that was sent, but we did not yet receive response to (while in reality it is a request that has not yet been sent).
Comment on lines
481
to
484
|
|
||
| // TODO: What should be the size of the channel? | ||
| let (sender, receiver) = mpsc::channel(1024); | ||
| let (sender, receiver) = mpsc::channel(config.pending_request_channel_size.unwrap_or(1024)); | ||
| let (error_sender, error_receiver) = tokio::sync::oneshot::channel(); |
There was a problem hiding this comment.
Please remove the TODO comment.
Also you are doing unwrap_or(1024), but you wrote in PR description that you changes the default to 2048 - it looks like a mistake.
Comment on lines
418
to
424
| tablet_sender: None, | ||
|
|
||
| identity: SelfIdentity::default(), | ||
| pending_request_channel_size: Some(2048), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The fact that in connection you wrote .unwrap_or(1024) (which is a bug) tells me that this approach is too error-prone.
Why use an option here? I don't see anything optional - we need to create the channel, and it needs to have some size. Let's make it just usize.
Comment on lines
449
to
453
|
|
||
| identity: SelfIdentity::default(), | ||
| pending_request_channel_size: Some(2048), | ||
| } | ||
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make pending request channel size configurable
Changes
Rationale
Usage
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.