fix: improve delay deletion #4800
Conversation
WalkthroughThe pull request modifies the default value of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (1)
src/service/compact/mod.rs (1)
Line range hint
204-220: Fix premature termination of background task due to dropped senderThe background task responsible for periodically updating job statuses may terminate immediately because the
_txsender is dropped shortly after creation. Since_txis not kept alive,rx.recv()returnsNoneimmediately, causing the task to exit before performing any updates.To fix this issue, retain the
_txsender in the outer scope to ensure it remains alive for the duration of the background task. Here is a suggested fix:let job_ids = jobs.iter().map(|job| job.id).collect::<Vec<_>>(); -let (_tx, mut rx) = mpsc::channel::<()>(1); +let (_tx, mut rx) = mpsc::channel::<()>(1); +let _tx_handle = _tx.clone(); // Keep a handle to the sender to prevent it from being dropped tokio::task::spawn(async move { loop { tokio::select! { _ = tokio::time::sleep(tokio::time::Duration::from_secs(ttl)) => {} _ = rx.recv() => { log::debug!("[COMPACT] update_running_jobs done"); return; } } for id in job_ids.iter() { if let Err(e) = infra_file_list::update_running_jobs(*id).await { log::error!("[COMPACT] update_job_status failed: {}", e); } } } }); +// Retain _tx_handle until the end of the function
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/config/src/config.rs (1 hunks)
- src/service/compact/mod.rs (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/config/src/config.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/compact/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
🔇 Additional comments (5)
src/config/src/config.rs (3)
Line range hint
1190-1194: Note: Existing safeguard for minimum batch sizeWhile not changed in this PR, it's worth noting that the
check_common_configfunction ensures a minimum value forbatch_size:if cfg.compact.batch_size < 1 { cfg.compact.batch_size = 100; }This safeguard prevents setting an invalid batch size. With the recent change to increase the default to 500, this check becomes less likely to be triggered but remains an important safety net.
Line range hint
1-1394: Summary: Configuration change to improve compaction performanceThis PR makes a targeted change to the
Compactstruct by increasing the defaultbatch_sizefrom 100 to 500. This modification aims to improve the efficiency of the delay deletion process by allowing more pending jobs to be processed in a single batch during compaction operations.Key points:
- The change is minimal and focused, affecting only one configuration parameter.
- Existing configuration checks and safeguards remain in place, ensuring the new default value is properly handled.
- The increased batch size has the potential to improve performance but should be monitored for any unintended consequences on memory usage or system responsiveness.
Overall, this change appears to be a reasonable optimization attempt. However, it's recommended to conduct thorough testing and monitoring in various scenarios to ensure it delivers the expected performance improvements without negative side effects.
1037-1037: Approved: Increased batch size for compact operationsThe change from 100 to 500 for the
batch_sizefield in theCompactstruct looks good. This increase should improve the efficiency of the compacting operation by processing more pending jobs in a single batch.To ensure this change doesn't negatively impact system performance, please monitor the following after deployment:
- Memory usage during compaction operations
- Overall system responsiveness
- Time taken for compaction operations to complete
You can use the following command to check for any logging or metrics related to compaction performance:
This will help identify any potential issues or confirm the positive impact of this change.
✅ Verification successful
Verified:
batch_sizedefault value updated to 500The change from
100to500for thebatch_sizefield in theCompactstruct has been successfully verified. This adjustment should enhance the efficiency of compacting operations by processing more jobs per batch.No removed or replaced code related to this change was detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg -i 'compact|batch_size' --type rustLength of output: 43273
Script:
#!/bin/bash # Find all assignments to 'batch_size' in Rust files rg -i 'batch_size\s*=' --type rustLength of output: 685
Script:
#!/bin/bash # Find all instances of 'batch_size' in Rust files, excluding those within strings rg -i 'batch_size' --type rust | grep -v '"'Length of output: 1920
src/service/compact/mod.rs (2)
Line range hint
64-71: Simplify node assignment logic using consistent hashingReplacing the distributed locking mechanism with
get_node_from_consistent_hashsimplifies the code and enhances performance by reducing overhead when determining the compactor node for each stream. This change improves maintainability.
375-381: Ensure consistent node assignment in delay deletionUsing
get_node_from_consistent_hashfor determining the compactor node in the delay deletion process aligns with the approach used in other functions. This provides consistency and reduces the complexity associated with distributed locks.
Reduce
disk_lockfor delay deletion job.Summary by CodeRabbit
New Features
Bug Fixes
Refactor