[ISSUE #6502]♻️Refactor transaction listener usage to utilize ArcTransactionListener for improved type safety and clarity

[mxsm]

Which Issue(s) This PR Fixes(Closes)

• Fixes [Refactor♻️] Refactor transaction listener usage to utilize ArcTransactionListener for improved type safety and clarity #6502

Brief Description

How Did You Test This Change?

Summary by CodeRabbit

• New Features

  • Added broker-side verification of transaction states for improved consistency
  • Introduced configurable thread pool parameters and request hold limits for transaction producers

• Improvements

  • Streamlined transaction listener configuration with sensible defaults

• Tests

  • Added test coverage for transaction producer builder configurations with custom and default settings

[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💥.

[coderabbitai]

Walkthrough

This pull request refactors transaction listener handling by introducing ArcTransactionListener as a simplified type alias for Arc<dyn TransactionListener>, eliminating unnecessary boxing. Concurrently, three new configuration options for thread pool sizing and request holding are added to transaction producer builders and configuration structures, with corresponding builder methods and sensible defaults.

Changes

Cohort / File(s)	Summary
Type Alias and Trait Extension rocketmq-client/src/producer/transaction_listener.rs	Introduces ArcTransactionListener type alias, adds check_local_transaction method to TransactionListener trait, and updates imports to support the new simplified type abstraction.
Type Signature Updates rocketmq-client/src/producer/producer_impl/default_mq_producer_impl.rs, rocketmq-client/src/producer/producer_impl/mq_producer_inner.rs	Updates method signatures to use ArcTransactionListener in place of Arc<Box<dyn TransactionListener>> for set_transaction_listener and get_check_listener; replaces tokio::spawn with spawn_blocking for potentially blocking transaction logic.
Configuration and Builder Enhancement rocketmq-client/src/producer/transaction_mq_produce_builder.rs, rocketmq-client/src/producer/transaction_mq_producer.rs	Adds check_thread_pool_min_size, check_thread_pool_max_size, and check_request_hold_max configuration fields with builder methods; updates listener storage to use ArcTransactionListener; adds explicit Default implementation for TransactionProducerConfig.
Test Coverage rocketmq-client/tests/transaction_producer_test.rs	Introduces new test file with three unit tests validating builder configurations and listener injection paths for transaction producers.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Hoppy refactoring hops along,
No boxing chains to make types wrong!
Arc and dyn dance light and free,
Thread pools tuned, configs agree,
Cleaner types, transaction strong! ✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Out of Scope Changes check	❓ Inconclusive	All changes are scoped to the transaction listener refactoring. However, new thread pool configuration fields were added beyond the core type alias change, which may extend beyond the basic refactoring scope.	Clarify whether thread pool configuration (check_thread_pool_min_size, check_thread_pool_max_size, check_request_hold_max) are required for the refactoring or represent scope expansion that should be a separate issue.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main refactoring objective: introducing ArcTransactionListener for improved type safety and clarity in transaction listener usage.
Linked Issues check	✅ Passed	The code changes successfully implement the refactoring objective from issue #6502: ArcTransactionListener type alias is introduced and integrated throughout the transaction listener API to replace Arc<Box>.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch refactor-6502

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

rocketmq-client/src/producer/transaction_mq_produce_builder.rs (1)

338-342: Consider clamping/validating min/max check thread-pool sizes

Currently any values (including 0 or min > max) are accepted. Normalizing in build avoids invalid configs.

♻️ Possible normalization in build

-        let transaction_producer_config = TransactionProducerConfig {
-            transaction_listener: self.transaction_listener,
-            check_thread_pool_min_size: self.check_thread_pool_min_size.unwrap_or(1),
-            check_thread_pool_max_size: self.check_thread_pool_max_size.unwrap_or(1),
-            check_request_hold_max: self.check_request_hold_max.unwrap_or(2000),
-        };
+        let min_pool = self.check_thread_pool_min_size.unwrap_or(1).max(1);
+        let max_pool = self.check_thread_pool_max_size.unwrap_or(min_pool).max(min_pool);
+        let transaction_producer_config = TransactionProducerConfig {
+            transaction_listener: self.transaction_listener,
+            check_thread_pool_min_size: min_pool,
+            check_thread_pool_max_size: max_pool,
+            check_request_hold_max: self.check_request_hold_max.unwrap_or(2000),
+        };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rocketmq-client/src/producer/transaction_mq_produce_builder.rs` around lines
338 - 342, The TransactionProducerConfig currently accepts any values for
check_thread_pool_min_size and check_thread_pool_max_size (including 0 or
min>max); update the builder's build method to validate and normalize these
fields before constructing TransactionProducerConfig: ensure
check_thread_pool_min_size is at least 1, ensure check_thread_pool_max_size is
at least check_thread_pool_min_size (or if max < min, set max = min), and clamp
both to sensible upper limits if desired (e.g., a safe constant) and also
validate check_request_hold_max (e.g., positive, default to 2000); perform these
adjustments where the TransactionProducerConfig is created so invalid configs
are never produced.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@rocketmq-client/src/producer/transaction_mq_produce_builder.rs`:
- Around line 338-342: The TransactionProducerConfig currently accepts any
values for check_thread_pool_min_size and check_thread_pool_max_size (including
0 or min>max); update the builder's build method to validate and normalize these
fields before constructing TransactionProducerConfig: ensure
check_thread_pool_min_size is at least 1, ensure check_thread_pool_max_size is
at least check_thread_pool_min_size (or if max < min, set max = min), and clamp
both to sensible upper limits if desired (e.g., a safe constant) and also
validate check_request_hold_max (e.g., positive, default to 2000); perform these
adjustments where the TransactionProducerConfig is created so invalid configs
are never produced.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 33efdf4 and 74e1f4c.

📒 Files selected for processing (6)

• rocketmq-client/src/producer/producer_impl/default_mq_producer_impl.rs
• rocketmq-client/src/producer/producer_impl/mq_producer_inner.rs
• rocketmq-client/src/producer/transaction_listener.rs
• rocketmq-client/src/producer/transaction_mq_produce_builder.rs
• rocketmq-client/src/producer/transaction_mq_producer.rs
• rocketmq-client/tests/transaction_producer_test.rs

[rocketmq-rust-bot]

LGTM - All CI checks passed ✅

[codecov]

Codecov Report

❌ Patch coverage is 18.84058% with 56 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.26%. Comparing base (33efdf4) to head (74e1f4c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...producer/producer_impl/default_mq_producer_impl.rs	0.00%	40 Missing ⚠️
...tmq-client/src/producer/transaction_mq_producer.rs	0.00%	9 Missing ⚠️
...ent/src/producer/transaction_mq_produce_builder.rs	68.42%	6 Missing ⚠️
...nt/src/producer/producer_impl/mq_producer_inner.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6503      +/-   ##
==========================================
+ Coverage   42.18%   42.26%   +0.07%     
==========================================
  Files         942      942              
  Lines      131785   131811      +26     
==========================================
+ Hits        55595    55707     +112     
+ Misses      76190    76104      -86     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.