[ISSUE #3534]🚀Implement connection handling in HAConnection methods with error logging#3535
[ISSUE #3534]🚀Implement connection handling in HAConnection methods with error logging#3535rocketmq-rust-bot merged 1 commit intomainfrom
Conversation
…ith error logging
rocketmq-rust-bot
requested review from
SpaceXCN,
TeslaRustor and
rocketmq-rust-bot
|
🔊@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
added
AI review first
WalkthroughThe implementation of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GeneralHAConnection
participant DefaultHAConnection
participant AutoSwitchHAConnection
Client->>GeneralHAConnection: Call HAConnection method
alt default_ha_connection is present
GeneralHAConnection->>DefaultHAConnection: Delegate method call
DefaultHAConnection-->>GeneralHAConnection: Return result
else auto_switch_ha_connection is present
GeneralHAConnection->>AutoSwitchHAConnection: Delegate method call
AutoSwitchHAConnection-->>GeneralHAConnection: Return result
else neither is present
GeneralHAConnection->>GeneralHAConnection: Log warning / error
GeneralHAConnection-->>Client: Return error or panic
end
Assessment against linked issues
Possibly related PRs
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.
Pull Request Overview
This PR implements connection handling for the GeneralHAConnection by replacing unimplemented todo!() stubs with logic that delegates to either the default or auto-switch HA connection, adds warning logs when no connection is available, and panics in getter methods if neither connection exists.
- Delegates
shutdown,close, and various getters to eitherdefault_ha_connectionorauto_switch_ha_connection - Adds
tracing::warn!logs on the(None, None)path - Introduces
panic!in getter methods when no HA connection is available
Comments suppressed due to low confidence (2)
rocketmq-store/src/ha/general_ha_connection.rs:107
- Avoid panicking in library code; consider returning a
ResultorOptionto propagate the absence of an HA connection instead of aborting execution.
panic!("No HA connection available");
rocketmq-store/src/ha/general_ha_connection.rs:78
- Add unit tests covering all match branches (especially the
None, Nonepath) to verify that warnings are logged and the behavior is correct.
async fn shutdown(&mut self) {
|
|
||
| async fn shutdown(&mut self) { | ||
| todo!() | ||
| match ( |
There was a problem hiding this comment.
[nitpick] The same match logic is repeated across multiple methods; consider extracting a helper function or macro to reduce duplication and improve maintainability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rocketmq-store/src/ha/general_ha_connection.rs (1)
78-165: Consider refactoring to reduce code duplication and improve maintainability.While the implementation correctly completes the trait requirements, the significant code duplication across getter methods could be reduced with a helper function or macro.
Consider extracting the common delegation pattern:
+ fn delegate_or_panic<T, F>(&self, operation_name: &str, operation: F) -> T + where + F: Fn(&DefaultHAConnection) -> T + Fn(&AutoSwitchHAConnection) -> T, + { + match (&self.default_ha_connection, &self.auto_switch_ha_connection) { + (Some(connection), _) => operation(connection), + (_, Some(connection)) => operation(connection), + (None, None) => { + tracing::warn!("No HA connection to {}", operation_name); + panic!("No HA connection available"); + } + } + }Also consider documenting that
default_ha_connectiontakes priority overauto_switch_ha_connectionwhen both are present.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rocketmq-store/src/ha/general_ha_connection.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
rocketmq-store/src/ha/general_ha_connection.rs (5)
rocketmq-store/src/ha/auto_switch/auto_switch_ha_connection.rs (7)
close(35-37)get_socket(39-41)get_current_state(43-45)get_client_address(47-49)get_transferred_byte_in_second(51-53)get_transfer_from_where(55-57)get_slave_ack_offset(59-61)rocketmq-store/src/ha/ha_connection.rs (7)
close(38-38)get_socket(44-44)get_current_state(50-50)get_client_address(56-56)get_transferred_byte_in_second(62-62)get_transfer_from_where(68-68)get_slave_ack_offset(74-74)rocketmq-store/src/ha/default_ha_connection.rs (7)
close(189-196)get_socket(198-200)get_current_state(202-204)get_client_address(206-208)get_transferred_byte_in_second(210-212)get_transfer_from_where(214-220)get_slave_ack_offset(222-224)rocketmq-store/src/ha/auto_switch/auto_switch_ha_client.rs (2)
get_current_state(60-62)get_transferred_byte_in_second(72-74)rocketmq-store/src/ha/general_ha_client.rs (2)
get_current_state(109-111)get_transferred_byte_in_second(121-123)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: test
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: auto-approve
- GitHub Check: build
🔇 Additional comments (2)
rocketmq-store/src/ha/general_ha_connection.rs (2)
78-89: Good implementation with proper async handling.The delegation logic is correct and handles the case where no connection is available appropriately. However, consider the inconsistency with the
startmethod which returns an error in similar circumstances.
91-99: Consistent implementation with appropriate warning logging.The synchronous delegation pattern matches the method signature and handles the no-connection case gracefully.
| fn get_socket(&self) -> &TcpStream { | ||
| todo!() | ||
| match (&self.default_ha_connection, &self.auto_switch_ha_connection) { | ||
| (Some(connection), _) => connection.get_socket(), | ||
| (_, Some(connection)) => connection.get_socket(), | ||
| (None, None) => { | ||
| tracing::warn!("No HA connection to get socket from"); | ||
| panic!("No HA connection available"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Verification correct
Inconsistent error handling and potential robustness concerns.
These getter methods use panic! when no connection is available, which is inconsistent with the graceful handling in shutdown/close methods. While panicking may be necessary given the trait return types, this approach reduces robustness in library code.
Additionally, there's significant code duplication across all these methods - they follow identical patterns.
Consider these alternatives:
- Return default/sentinel values where semantically appropriate
- Modify the trait to return
Resulttypes for better error handling - Ensure underlying implementations are complete before relying on them
Let me verify if the underlying implementations are ready:
Also applies to: 112-121, 123-132, 134-143, 145-154, 156-165
🏁 Script executed:
#!/bin/bash
# Verify if underlying implementations are complete
echo "Checking DefaultHAConnection implementations:"
rg -A 5 "fn get_socket" rocketmq-store/src/ha/default_ha_connection.rs
echo -e "\nChecking AutoSwitchHAConnection implementations:"
rg -A 5 "fn get_socket\|fn get_client_address\|fn get_transferred_byte_in_second" rocketmq-store/src/ha/auto_switch/auto_switch_ha_connection.rsLength of output: 599
Inconsistent error handling and potential robustness concerns.
These getter methods use panic! when no connection is available, which is inconsistent with the graceful handling in shutdown/close methods. While panicking may be necessary given the trait return types, this approach reduces robustness in library code.
Additionally, there's significant code duplication across all these methods - they follow identical patterns.
Consider these alternatives:
- Return default/sentinel values where semantically appropriate
- Modify the trait to return
Resulttypes for better error handling - Ensure underlying implementations are complete before relying on them
Let me verify if the underlying implementations are ready:
#!/bin/bash
# Verify if underlying implementations are complete
echo "Checking DefaultHAConnection implementations:"
rg -A 5 "fn get_socket" rocketmq-store/src/ha/default_ha_connection.rs
echo -e "\nChecking AutoSwitchHAConnection implementations:"
rg -A 5 "fn get_socket\|fn get_client_address\|fn get_transferred_byte_in_second" rocketmq-store/src/ha/auto_switch/auto_switch_ha_connection.rsAlso applies to: 112-121, 123-132, 134-143, 145-154, 156-165
🤖 Prompt for AI Agents
In rocketmq-store/src/ha/general_ha_connection.rs around lines 101 to 110, the
get_socket method panics if no connection is available, which is inconsistent
with the graceful error handling in shutdown/close methods and reduces
robustness. Refactor these getter methods to reduce code duplication by
extracting the common pattern into a helper function that returns an Option or
Result, depending on trait constraints. If possible, modify the trait to return
Result types for better error handling; otherwise, consider returning default or
sentinel values where appropriate. Also, verify that the underlying
DefaultHAConnection and AutoSwitchHAConnection implementations are complete
before relying on them to avoid panics.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3535 +/- ##
==========================================
- Coverage 26.18% 26.17% -0.02%
==========================================
Files 556 556
Lines 78642 78681 +39
==========================================
Hits 20593 20593
- Misses 58049 58088 +39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #3534
Brief Description
How Did You Test This Change?
Summary by CodeRabbit