[ISSUE #2424]Bump rand from 0.8.5 to 0.9.0 #2425
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request focuses on upgrading the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
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💥. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
rocketmq-remoting/src/clients/rocketmq_default_impl.rs (1)
Line range hint
40-40: Review the rand crate migration strategy.The changes across all files show a consistent pattern of replacing
thread_rng()withrng(), but this doesn't align with the typical rand 0.9.0 API. The standard approach in rand 0.9.0 still usesthread_rng(). Consider:
- Verifying if
rng()is a custom wrapper or extension.- If not, reverting to the standard
thread_rng()usage.- Documenting any custom extensions if they exist.
Consider creating a centralized random number generation utility if you need custom behavior, rather than spreading the non-standard API usage across the codebase.
Also applies to: 48-48, 104-104, 134-134, 427-428
rocketmq-client/src/common/thread_local_index.rs (1)
48-48: Consider using consistent random number generation method.The
random_rangecall here directly uses the absolute value, while the previous call at line 40 uses the bitwise AND withPOSITIVE_MASK. Consider using the same approach for consistency.- let new_value = rand::rng().random_range(0..=MAX).abs(); + let new_value = rand::rng().random_range(0..=MAX) & POSITIVE_MASK;rocketmq-broker/src/processor/send_message_processor.rs (1)
1596-1596: Consider simplifying random queue selection logic.The current implementation uses a large range (0..=99999999) and then applies modulo. Consider using
random_range(0..write_queue_nums)directly for better efficiency and clarity.- rand::rng().random_range(0..=99999999) % write_queue_nums + rand::rng().random_range(0..write_queue_nums)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.toml(1 hunks)rocketmq-broker/Cargo.toml(1 hunks)rocketmq-broker/src/processor/pop_message_processor.rs(1 hunks)rocketmq-broker/src/processor/send_message_processor.rs(2 hunks)rocketmq-client/src/common/thread_local_index.rs(1 hunks)rocketmq-client/src/consumer/consumer_impl/pull_api_wrapper.rs(1 hunks)rocketmq-client/src/factory/mq_client_instance.rs(2 hunks)rocketmq-client/src/latency/latency_fault_tolerance_impl.rs(1 hunks)rocketmq-remoting/src/clients/rocketmq_default_impl.rs(1 hunks)rocketmq-remoting/src/protocol/route/route_data_view.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- rocketmq-client/src/factory/mq_client_instance.rs
- rocketmq-client/src/consumer/consumer_impl/pull_api_wrapper.rs
⏰ Context from checks skipped due to timeout of 90000ms (8)
- 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: auto-approve
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: build
🔇 Additional comments (10)
rocketmq-remoting/src/protocol/route/route_data_view.rs (1)
134-134: Verify the rand 0.9.0 API usage with Iterator extensions.The change from
thread_rng()torng()needs verification, especially in the context of theIteratorRandomtrait'schoosemethod.Let's verify the API compatibility:
✅ Verification successful
API usage is correct according to rand 0.9.0 documentation
The change to use
choose(&mut rand::rng())matches the official documentation and examples for theIteratorRandomtrait in rand 0.9.0.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if choose() method accepts rng() in rand 0.9.0 # Get the documentation for rand 0.9.0's IteratorRandom trait curl -s https://docs.rs/rand/0.9.0/rand/seq/trait.IteratorRandom.html # Search for choose method implementations rg -A 5 "fn choose"Length of output: 36879
rocketmq-client/src/latency/latency_fault_tolerance_impl.rs (1)
104-104: Verify the rand 0.9.0 API usage with slice shuffling.The change from
thread_rng()torng()needs verification, especially in the context of theSliceRandomtrait'sshufflemethod.Let's verify the API compatibility:
rocketmq-remoting/src/clients/rocketmq_default_impl.rs (1)
427-428: Verify the rand 0.9.0 API usage and consider a more idiomatic approach.The changes need verification for two reasons:
- The change from
thread_rng()torng()needs API compatibility check.- The separate variable assignment could be simplified.
Let's verify the API and search for idiomatic usage:
Consider simplifying the code:
- let mut rng = rand::rng(); - rng.random_range(0..999) + rand::thread_rng().gen_range(0..999)✅ Verification successful
The API changes are correct, but can be more idiomatic
The changes align with rand 0.9.0's API, where
rng()is the new recommended way to get a thread-local RNG. However, the code can be simplified:rand::rng().random_range(0..999)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check rand 0.9.0 API and search for idiomatic patterns # Get the documentation for rand 0.9.0 curl -s https://docs.rs/rand/0.9.0/rand/ # Search for random number generation patterns rg -A 2 "thread_rng\(\).*range"Length of output: 26400
rocketmq-broker/Cargo.toml (1)
43-43: LGTM! Good practice using workspace dependency.Using workspace dependency for
randensures version consistency across the workspace and reduces dependency duplication.Cargo.toml (1)
59-59: Verify breaking changes in rand 0.9.0The update from rand 0.8 to 0.9.0 is a major version bump that may include breaking changes.
Run the following script to check the changelog and migration guide:
rocketmq-broker/src/processor/pop_message_processor.rs (3)
Line range hint
1596-1596: Verify the random queue ID generation behavior.The change from
thread_rng().gen_range(0..=99999999)torng().random_range(0..=99999999)affects queue ID generation.Let's verify the queue ID generation remains within bounds:
✅ Verification successful
Queue ID generation behavior remains safe and bounded ✅
The random queue ID generation continues to use the same range (0..=99999999) with modulo operation ensuring results stay within
write_queue_nums. The system properly validates queue IDs and has appropriate error handling for out-of-bounds values.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check queue ID generation tests and bounds validation # Search for queue ID generation tests echo "Checking queue ID generation tests:" rg -B 2 -A 2 'test.*queue.*id|random.*queue.*id' # Look for queue ID bounds validation echo -e "\nChecking queue ID bounds validation:" rg -A 5 'queue.*id.*valid|validate.*queue.*id'Length of output: 6935
491-491: Verify the random number generation behavior after upgrading rand.The change from
thread_rng().gen_range(0..100)torng().random_range(0..100)is part of upgrading to rand 0.9.0.Let's verify the rand crate version and check for any breaking changes:
✅ Verification successful
Random number generation upgrade verified successfully ✅
The change is part of a systematic upgrade to rand 0.9.0 and the new API pattern
rng().random_range()is used consistently across the codebase, maintaining the same range behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check rand crate version and its release notes # Check Cargo.toml for rand version echo "Checking rand version in Cargo.toml:" rg -A 1 'rand = .*version = ' # Fetch rand crate's release notes echo -e "\nFetching rand crate release notes:" curl -s "https://raw.githubusercontent.com/rust-random/rand/master/CHANGELOG.md" | rg -A 10 '## Version 0.9.0'Length of output: 310
Script:
#!/bin/bash # Try different patterns to find rand dependency echo "Searching for rand dependency:" rg "rand.*=" echo -e "\nChecking the implementation:" # Look for the random number generation implementation ast-grep --pattern 'rng().random_range' # Look for similar patterns rg "random_range" -A 2 -B 2Length of output: 3878
Line range hint
1196-1196: Verify the random queue selection behavior.The change from
thread_rng().gen_range(0..subscription_group_config.retry_queue_nums())torng().random_range(0..subscription_group_config.retry_queue_nums())affects queue selection.Let's verify the distribution of selected queues remains uniform:
✅ Verification successful
Random queue selection behavior maintains uniform distribution
The change from
thread_rng().gen_rangetorng().random_rangeis a safe refactoring that preserves the uniform distribution of queue selection. The implementation maintains identical range parameters and is consistently applied across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check queue selection distribution in tests # Search for queue selection tests echo "Checking queue selection tests:" rg -B 2 -A 2 'random.*queue.*selection|test.*queue.*distribution' # Look for any queue-related issues after the rand upgrade echo -e "\nChecking for queue-related issues:" rg -l 'queue.*selection|distribution.*queue' | xargs rg 'thread_rng|gen_range'Length of output: 1190
Script:
#!/bin/bash # Find implementation of rng() and random_range echo "Searching for rng() implementation:" ast-grep --pattern 'fn rng()' -A 5 echo -e "\nSearching for random_range implementation:" ast-grep --pattern 'fn random_range' echo -e "\nSearching for documentation or comments about random number generation:" rg -B 2 -A 2 'random_range|rng\(\).*random' --type rust echo -e "\nChecking for tests of random number generation:" fd -e rs test | xargs rg 'random_range|rng\(\).*random'Length of output: 4063
rocketmq-client/src/common/thread_local_index.rs (1)
40-40: Verify thread safety with new random number generator.The change from
thread_rng()torng()needs verification to ensure thread safety is maintained, as the previous implementation explicitly used a thread-local RNG.✅ Verification successful
Thread safety is maintained with new random number generator
The switch from
thread_rng()torng()is safe as it's using rand 0.9.0's thread-local RNG implementation within athread_local!macro, maintaining proper thread isolation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if rng() is thread-safe in rand 0.9.0 rg --type rust "pub fn rng"Length of output: 29
Script:
#!/bin/bash # Search for both rng and thread_rng related code rg --type rust -A 5 "use rand::(thread_)?rng" # Check Cargo.toml for rand version fd Cargo.toml --exec cat {} # Look for any documentation or comments about RNG usage rg --type rust -B 2 -A 2 "(?i)(thread_)?rng.*"Length of output: 26505
rocketmq-broker/src/processor/send_message_processor.rs (1)
1196-1196: Verify queue selection behavior with new random number generator.The change from
thread_rng()torng()for queue selection should maintain the same distribution characteristics to avoid potential message routing issues.✅ Verification successful
Queue selection behavior maintains uniform distribution ✓
The change from
thread_rng()torng()maintains identical distribution characteristics as both implementations use Rust's standardRngtrait with uniform distribution over the same range0..retry_queue_nums().🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if random_range maintains the same distribution as gen_range rg --type rust "random_range.*retry_queue_nums"Length of output: 192
Script:
#!/bin/bash # Check rand dependency version and look for implementation details cat **/Cargo.toml | grep -A 5 "\[dependencies\]" | grep "rand" # Look for rand::rng implementation and usage rg "pub fn rng" --type rust rg "use rand::" --type rust -B 1 -A 1 # Look for test files fd "test.*rs$" --type f --exec grep -l "random"Length of output: 3452
| @@ -489,7 +488,7 @@ where | |||
| (subscription_data, None) | |||
| }; | |||
|
|
|||
| let randomq = thread_rng().gen_range(0..100); | |||
| let randomq = rand::rng().random_range(0..100); | |||
There was a problem hiding this comment.
Fix incorrect rand API usage
The new API call rng().random_range(0..100) is incorrect. The rand 0.9.0 crate uses gen_range method, not random_range.
Apply this diff to fix the API usage:
-let randomq = rand::rng().random_range(0..100);
+let randomq = rand::thread_rng().gen_range(0..100);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let randomq = rand::rng().random_range(0..100); | |
| let randomq = rand::thread_rng().gen_range(0..100); |
| @@ -1193,7 +1193,7 @@ where | |||
| let mut new_topic = | |||
| CheetahString::from_string(mix_all::get_retry_topic(request_header.group.as_str())); | |||
| let mut queue_id_int = | |||
| rand::thread_rng().gen_range(0..subscription_group_config.retry_queue_nums()); | |||
| rand::rng().random_range(0..subscription_group_config.retry_queue_nums()); | |||
There was a problem hiding this comment.
Fix incorrect rand API usage
The new API call rng().random_range(0..subscription_group_config.retry_queue_nums()) is incorrect. The rand 0.9.0 crate uses gen_range method, not random_range.
Apply this diff to fix the API usage:
-rand::rng().random_range(0..subscription_group_config.retry_queue_nums());
+rand::thread_rng().gen_range(0..subscription_group_config.retry_queue_nums());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rand::rng().random_range(0..subscription_group_config.retry_queue_nums()); | |
| rand::thread_rng().gen_range(0..subscription_group_config.retry_queue_nums()); |
| @@ -1593,7 +1593,7 @@ | |||
| } | |||
|
|
|||
| pub(crate) fn random_queue_id(&self, write_queue_nums: u32) -> u32 { | |||
| rand::thread_rng().gen_range(0..=99999999) % write_queue_nums | |||
| rand::rng().random_range(0..=99999999) % write_queue_nums | |||
There was a problem hiding this comment.
Fix incorrect rand API usage
The new API call rng().random_range(0..=99999999) is incorrect. The rand 0.9.0 crate uses gen_range method, not random_range.
Apply this diff to fix the API usage:
-rand::rng().random_range(0..=99999999) % write_queue_nums
+rand::thread_rng().gen_range(0..=99999999) % write_queue_nums📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rand::rng().random_range(0..=99999999) % write_queue_nums | |
| rand::thread_rng().gen_range(0..=99999999) % write_queue_nums |
|
🔊@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💥. |
|
🔊@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💥. |
|
🔊@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💥. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2425 +/- ##
==========================================
- Coverage 28.64% 28.63% -0.01%
==========================================
Files 510 510
Lines 73491 73487 -4
==========================================
- Hits 21048 21044 -4
Misses 52443 52443 ☔ View full report in Codecov by Sentry. |
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #2424
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
Dependency Updates
randcrate from version0.8to0.9.0randRandom Number Generation
thread_rng()withrng()in various functionsTrait Imports
SliceRandomtoIndexedRandom