[ISSUE #2349]🤡Connection supports send bytes 🧑💻 #2352
Conversation
WalkthroughThe pull request introduces significant changes to the RocketMQ remoting module, focusing on refactoring command handling and communication mechanisms. The primary modifications involve replacing the existing command transmission methods with new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
|
|
🔊@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💥. |
rocketmq-rust-bot
requested review from
rocketmq-rust-bot,
SpaceXCN and
TeslaRustor
rocketmq-rust-bot
added
AI review first
RocketmqRustBot
added
feature🚀
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
rocketmq-remoting/src/connection.rs (1)
65-68: Consider derivingHashandPartialEqinstead of manual implementationsThe manual implementations of
HashandPartialEquse the addresses ofwriterandreader, which may not provide consistent behavior and could lead to unexpected results due to pointer uniqueness rather than content. Deriving these traits automatically ensures that hashing and equality checks are based on the content of the fields and reduces the risk of errors.Apply this diff to derive
Hash,PartialEq, andEqtraits:+#[derive(Hash, PartialEq, Eq)] pub struct Connection { /// The `Framed` instance used for reading from and writing to the TCP stream. //pub(crate) framed: Framed<TcpStream, RemotingCommandCodec>, writer: SplitSink<Framed<TcpStream, CompositeCodec>, BytesMut>, reader: SplitStream<Framed<TcpStream, CompositeCodec>>, /// A boolean flag indicating the current state of the connection. /// `true` means the connection is in a good state, while `false` indicates /// there are issues with the connection. pub(crate) ok: bool, } -impl Hash for Connection { - fn hash<H: Hasher>(&self, state: &mut H) { - // Hash the boolean flag - self.ok.hash(state); - - // Use the addresses of writer and reader to hash them (they serve as a unique - // identifier for these components) - let writer_addr: *const SplitSink<Framed<TcpStream, CompositeCodec>, BytesMut> = - &self.writer as *const SplitSink<Framed<TcpStream, CompositeCodec>, BytesMut>; - let reader_addr: *const SplitStream<Framed<TcpStream, CompositeCodec>> = - &self.reader as *const SplitStream<Framed<TcpStream, CompositeCodec>>; - - writer_addr.hash(state); - reader_addr.hash(state); - } -} - -impl PartialEq for Connection { - fn eq(&self, other: &Self) -> bool { - // Compare the boolean flag - self.ok == other.ok - - // Compare the addresses of writer and reader - && (std::ptr::eq(&self.writer, &other.writer)) - && (std::ptr::eq(&self.reader, &other.reader)) - } -} - -impl Eq for Connection {}rocketmq-remoting/src/remoting_server/server.rs (2)
Line range hint
185-196: Fix spacing in select macro.There's inconsistent spacing in the select macro. Add a space after
result =.- result =self.connection_handler_context.channel.connection.send_command(response.set_opaque(opaque)) + result = self.connection_handler_context.channel.connection.send_command(response.set_opaque(opaque))
Line range hint
217-238: Consider refactoring duplicated error handling.The error handling pattern is duplicated across multiple select blocks. Consider extracting it into a helper method.
async fn handle_send_error(&mut self, err: RemotingError) -> HandleErrorResult { match err { RemotingError::Io(io_error) => { error!("send response failed: {}", io_error); HandleErrorResult::ReturnMethod } _ => { error!("send response failed: {}", err); HandleErrorResult::Continue } } }Also, fix the spacing in the select macros:
- result =self.connection_handler_context.channel.connection.send_command(response.set_opaque(opaque)) + result = self.connection_handler_context.channel.connection.send_command(response.set_opaque(opaque))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
rocketmq-remoting/src/clients/client.rs(3 hunks)rocketmq-remoting/src/connection.rs(4 hunks)rocketmq-remoting/src/net/channel.rs(1 hunks)rocketmq-remoting/src/remoting_server/server.rs(4 hunks)rocketmq-remoting/src/runtime/connection_handler_context.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: build
- GitHub Check: test
- GitHub Check: auto-approve
- GitHub Check: build (ubuntu-latest, stable)
🔇 Additional comments (7)
rocketmq-remoting/src/connection.rs (1)
123-147: New methodsreceive_commandandsend_commandenhance abstractionThe addition of
receive_commandandsend_commandmethods improves the abstraction for sending and receiving commands over the connection. This encapsulates the underlying implementation details and enhances code readability and maintainability.rocketmq-remoting/src/runtime/connection_handler_context.rs (1)
Line range hint
47-51: Update tosend_commandimproves error handling consistencyUsing the
send_commandmethod aligns the command-sending mechanism with the updatedConnectionAPI and ensures consistent error handling across the codebase.rocketmq-remoting/src/net/channel.rs (1)
Line range hint
69-83: Refactored to usesend_commandmethod for command transmissionUpdating to use
send_commandmethod in therun_sendfunction simplifies the code and leverages the encapsulated sending logic withinConnection, improving maintainability.rocketmq-remoting/src/clients/client.rs (3)
72-72: LGTM! Improved method naming.The change from
reader.next()toreceive_command()better encapsulates the command reception logic and improves code readability.
194-194: LGTM! Consistent method naming.The change to
send_command()maintains consistency with the receive_command pattern and better represents the method's purpose.
327-327: LGTM! Consistent error handling.The change to
receive_command()maintains consistency with other changes while preserving robust error handling for connection issues.rocketmq-remoting/src/remoting_server/server.rs (1)
114-114: LGTM! Consistent with client implementation.The change to
receive_command()aligns with the client-side changes while maintaining proper shutdown handling.
rocketmq-rust-bot
added
approved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2352 +/- ##
==========================================
- Coverage 28.41% 28.41% -0.01%
==========================================
Files 505 505
Lines 72835 72847 +12
==========================================
Hits 20699 20699
- Misses 52136 52148 +12 ☔ View full report in Codecov by Sentry. |
Which Issue(s) This PR Fixes(Closes)
Fixes #2349
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
Release Notes
Refactor
Performance
Technical Improvements