Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[ISSUE #1484]♻️Refactor create MQBrokerErr replace with client_broker_err! macro🔥 #1486

Merged
merged 1 commit into from
Dec 1, 2024
Merged

[ISSUE #1484]♻️Refactor create MQBrokerErr replace with client_broker_err! macro🔥 #1486

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 40 additions & 45 deletions rocketmq-client/src/implementation/mq_client_api_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
use tracing::warn;

use crate::base::client_config::ClientConfig;
use crate::client_broker_err;
use crate::client_error::MQBrokerErr;
use crate::client_error::MQClientError;
use crate::client_error::MQClientError::MQClientBrokerError;
Expand Down Expand Up @@ -683,13 +684,11 @@
if ResponseCode::from(response.code()) == ResponseCode::Success {
return Ok(response.version());
}
Err(MQClientError::MQClientBrokerError(
MQBrokerErr::new_with_broker(
response.code(),
response.remark().map_or("".to_string(), |s| s.to_string()),
addr.to_string(),
),
))
client_broker_err!(
response.code(),
response.remark().map_or("".to_string(), |s| s.to_string()),
addr.to_string()
)

Check warning on line 691 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L687-L691 

Added lines #L687 - L691 were not covered by tests
Comment on lines +687 to +691
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Inconsistent error handling pattern found - some instances still use direct constructor

The verification revealed that there are still 4 instances in mq_client_api_impl.rs where MQBrokerErr is being created directly instead of using the client_broker_err! macro. These instances should be refactored for consistency:

  • Line ~200: MQBrokerErr::new_with_broker in SendStatus match block
  • Line ~300: MQBrokerErr::new_with_broker in another match block
  • Line ~400: MQBrokerErr::new_with_broker in response code check
  • Line ~500: MQBrokerErr::new_with_broker in LockBatchResponseBody decode error handling
🔗 Analysis chain

Consistent refactoring of error handling using the new macro.

The replacement of direct MQBrokerErr creation with the client_broker_err! macro has been consistently applied across all error handling cases. This refactoring:

  • Simplifies error creation
  • Reduces code duplication
  • Maintains consistent error handling patterns
  • Preserves the same error information (code, remark, broker address)

Let's verify that all instances of MQBrokerErr creation have been replaced:

Also applies to: 779-783, 870-874, 970-974, 1037-1041, 1068-1072, 1108-1112, 1151-1155, 1158-1162, 1222-1226, 1308-1312

Let me check the remaining instances of direct MQBrokerErr creation in the implementation file to verify if they should be using the macro.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining direct MQBrokerErr creation that might have been missed

# Search for any remaining direct MQBrokerErr::new calls
rg "MQBrokerErr::new" 

# Search for any remaining direct MQBrokerErr::new_with_broker calls
rg "MQBrokerErr::new_with_broker"

Length of output: 1604

Script:

#!/bin/bash
# Get the context around the remaining direct MQBrokerErr usages in the implementation
rg -B 3 -A 3 "MQBrokerErr::new_with_broker" rocketmq-client/src/implementation/mq_client_api_impl.rs

Length of output: 1486

}

pub async fn check_client_in_broker(
Expand Down Expand Up @@ -777,13 +776,11 @@
));
}
}
Err(MQClientError::MQClientBrokerError(
MQBrokerErr::new_with_broker(
response.code(),
response.remark().map_or("".to_string(), |s| s.to_string()),
addr.to_string(),
),
))
client_broker_err!(
response.code(),
response.remark().map_or("".to_string(), |s| s.to_string()),
addr.to_string()
)

Check warning on line 783 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L779-L783 

Added lines #L779 - L783 were not covered by tests
}

pub async fn update_consumer_offset_oneway(
Expand Down Expand Up @@ -870,13 +867,11 @@
}
_ => {}
}
Err(MQClientError::MQClientBrokerError(
MQBrokerErr::new_with_broker(
response.code(),
response.remark().map_or("".to_string(), |s| s.to_string()),
addr.to_string(),
),
))
client_broker_err!(
response.code(),
response.remark().map_or("".to_string(), |s| s.to_string()),
addr.to_string()
)

Check warning on line 874 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L870-L874 

Added lines #L870 - L874 were not covered by tests
}

pub async fn pull_message<PCB>(
Expand Down Expand Up @@ -972,11 +967,11 @@
ResponseCode::PullRetryImmediately => PullStatus::NoMatchedMsg,
ResponseCode::PullOffsetMoved => PullStatus::OffsetIllegal,
_ => {
return Err(MQClientBrokerError(MQBrokerErr::new_with_broker(
return client_broker_err!(

Check warning on line 970 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L970 

Added line #L970 was not covered by tests
response.code(),
response.remark().map_or("".to_string(), |s| s.to_string()),
addr.to_string(),
)))
addr.to_string()
)

Check warning on line 974 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L973-L974 

Added lines #L973 - L974 were not covered by tests
}
};
let response_header = response
Expand Down Expand Up @@ -1039,11 +1034,11 @@
if ResponseCode::from(response.code()) == ResponseCode::Success {
Ok(())
} else {
Err(MQClientBrokerError(MQBrokerErr::new_with_broker(
client_broker_err!(

Check warning on line 1037 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L1037 

Added line #L1037 was not covered by tests
response.code(),
response.remark().map_or("".to_string(), |s| s.to_string()),
addr.to_string(),
)))
addr.to_string()
)

Check warning on line 1041 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L1040-L1041 

Added lines #L1040 - L1041 were not covered by tests
}
}

Expand All @@ -1070,11 +1065,11 @@
if ResponseCode::from(response.code()) == ResponseCode::Success {
Ok(())
} else {
Err(MQClientBrokerError(MQBrokerErr::new_with_broker(
client_broker_err!(

Check warning on line 1068 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L1068 

Added line #L1068 was not covered by tests
response.code(),
response.remark().map_or("".to_string(), |s| s.to_string()),
addr.to_string(),
)))
addr.to_string()
)

Check warning on line 1072 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L1071-L1072 

Added lines #L1071 - L1072 were not covered by tests
}
}

Expand Down Expand Up @@ -1110,11 +1105,11 @@
if ResponseCode::from(response.code()) == ResponseCode::Success {
Ok(())
} else {
Err(MQClientBrokerError(MQBrokerErr::new_with_broker(
client_broker_err!(

Check warning on line 1108 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L1108 

Added line #L1108 was not covered by tests
response.code(),
response.remark().map_or("".to_string(), |s| s.to_string()),
addr.to_string(),
)))
addr.to_string()
)

Check warning on line 1112 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L1111-L1112 

Added lines #L1111 - L1112 were not covered by tests
}
}
}
Expand Down Expand Up @@ -1153,18 +1148,18 @@
))
})
} else {
Err(MQClientBrokerError(MQBrokerErr::new_with_broker(
client_broker_err!(

Check warning on line 1151 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L1151 

Added line #L1151 was not covered by tests
response.code(),
"Response body is empty".to_string(),
addr.to_string(),
)))
addr.to_string()
)

Check warning on line 1155 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L1154-L1155 

Added lines #L1154 - L1155 were not covered by tests
}
} else {
Err(MQClientBrokerError(MQBrokerErr::new_with_broker(
client_broker_err!(

Check warning on line 1158 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L1158 

Added line #L1158 was not covered by tests
response.code(),
response.remark().map_or("".to_string(), |s| s.to_string()),
addr.to_string(),
)))
addr.to_string()
)

Check warning on line 1162 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L1161-L1162 

Added lines #L1161 - L1162 were not covered by tests
}
}

Expand Down Expand Up @@ -1224,11 +1219,11 @@
.expect("decode error");
return Ok(response_header.offset);
}
Err(MQClientBrokerError(MQBrokerErr::new_with_broker(
client_broker_err!(

Check warning on line 1222 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L1222 

Added line #L1222 was not covered by tests
response.code(),
response.remark().map_or("".to_string(), |s| s.to_string()),
addr.to_string(),
)))
addr.to_string()
)

Check warning on line 1226 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L1225-L1226 

Added lines #L1225 - L1226 were not covered by tests
}

pub async fn set_message_request_mode(
Expand Down Expand Up @@ -1310,10 +1305,10 @@
return Ok(None);
}

Err(MQClientBrokerError(MQBrokerErr::new_with_broker(
client_broker_err!(

Check warning on line 1308 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L1308 

Added line #L1308 was not covered by tests
response.code(),
response.remark().map_or("".to_string(), |s| s.to_string()),
addr.to_string(),
)))
addr.to_string()
)

Check warning on line 1312 in rocketmq-client/src/implementation/mq_client_api_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-client/src/implementation/mq_client_api_impl.rs#L1311-L1312 

Added lines #L1311 - L1312 were not covered by tests
}
}
Loading