[ISSUE #3521]⚡️Increase buffer capacity for Framed connection to 1 MB in new Connection implementation#3522
Conversation
… in new Connection implementation
There was a problem hiding this comment.
Pull Request Overview
This PR increases the buffer capacity for the Framed connection in the new Connection implementation to 1 MB as described in Issue #3521.
- Introduces a constant CAPACITY set to 1 MB.
- Updates the Framed instance initialization to use the new CAPACITY.
|
🔊@mxsm 🚀Thanks for your contribution🎉! 💡CodeRabbit(AI) will review your code first🔥! Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
WalkthroughThe internal buffer capacity for framed connections in the Changes
Assessment against linked issues
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rocketmq-remoting/src/connection.rs (1)
101-103: Make buffer capacity a shared, configurable constant instead of an in-functionconstHard-coding
1 MBinside thenew()constructor makes it difficult to reason about memory usage across many connections and impossible to tweak the value from a single place (or via a feature flag / config file) without touching this method.- const CAPACITY: usize = 1024 * 1024; // 1 MB - let framed = Framed::with_capacity(tcp_stream, CompositeCodec::new(), CAPACITY); + // Prefer a module-level or crate-level constant so it can be reused and + // adjusted centrally. + let framed = + Framed::with_capacity(tcp_stream, CompositeCodec::new(), BUFFER_CAPACITY_BYTES);Suggested follow-up:
+/// Default framed buffer size per connection (1 MB). +pub(crate) const BUFFER_CAPACITY_BYTES: usize = 1024 * 1024;Also consider exposing a knob (e.g. via builder or config) so applications can reduce the per-connection footprint when running thousands of concurrent connections.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rocketmq-remoting/src/connection.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: test
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: auto-approve
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3522 +/- ##
=======================================
Coverage 26.20% 26.20%
=======================================
Files 555 555
Lines 78594 78594
=======================================
Hits 20593 20593
Misses 58001 58001 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Which Issue(s) This PR Fixes(Closes)
Fixes #3521
Brief Description
How Did You Test This Change?
Summary by CodeRabbit