feat: introduce the interface of RemoteJobScheduler

[zyy17]

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

What's changed and what's your intention?

RemoteJobScheduler

For the storage disaggregated system, we can always offload the CPU-intensive and IO-intensive tasks(for example, compaction and index) to the remote workers. For the above scenario, the PR introduces the abstraction.

RemoteJobScheduler is a trait that defines the API for scheduling remote jobs. Its implementation is in GreptimeDB Enterprise.

/// RemoteJobScheduler is a trait that defines the API to schedule remote jobs.
#[async_trait::async_trait]
pub trait RemoteJobScheduler: Send + Sync + 'static {
    /// Sends a job to the scheduler and returns a unique identifier for the job.
    async fn schedule(&self, job: RemoteJob, notifier: Arc<dyn Notifier>) -> Result<JobId>;
}

/// Notifier is used to notify the mito engine when a remote job is completed.
#[async_trait::async_trait]
pub trait Notifier: Send + Sync + 'static {
    /// Notify the mito engine that a remote job is completed.
    async fn notify(&self, result: RemoteJobResult);
}

The PR modify schedule_compaction_request() to support remote compaction:

• If the compact request specifies the remote_compaction in region_options and the RemoteJobScheduler is initialized, the Mito will execute remote compaction;
• If the remote compaction fails, fall back to local compaction;

Other changes

• Add the async keyword for all the compaction-related functions because the schedule_compaction_request needs to be async;

• Use Option type for senders in CompactionFinished because we don't need it in remote compaction scenario;

• Add remote_compaction in compaction options;

TODOs

• Inject RemoteJobScheduler from the plugin system;
• Design the API to fetch the Jobs from the scheduler. When the datanode restarts, it can rebuild the context of the remote job;
• Add the unit tests for the RemoteJobScheduler;

Checklist

• I have written the necessary rustdoc comments.
• I have added the necessary unit tests and integration tests.
• This PR requires documentation updates.

Summary by CodeRabbit

• New Features

  • Introduced support for remote compaction scheduling.
  • Added Plugins functionality to enhance compaction scheduling and handling.
  • Enabled asynchronous operations for scheduling and finishing compaction tasks.
  • Integrated remote job scheduling mechanics for compaction.

• Enhancements

  • Updated MitoEngine and EngineInner constructors to accept plugins parameters.
  • Improved error handling with new RemoteJobScheduler and other error variants.
  • Added checks for remote_compaction in compaction options.

• Bug Fixes

  • Improved test framework to handle new plugin arguments and asynchronous calls.

[coderabbitai]

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 613edc8 and d299592.

Files selected for processing (4)

• src/mito2/src/compaction.rs (16 hunks)
• src/mito2/src/error.rs (5 hunks)
• src/mito2/src/request.rs (1 hunks)
• src/mito2/src/schedule/remote_job_scheduler.rs (1 hunks)

Files skipped from review as they are similar to previous changes (1)

• src/mito2/src/error.rs

Additional comments not posted (7)

src/mito2/src/schedule/remote_job_scheduler.rs (1)

60-64: Consider modifying the Notifier.notify method to return a Result.

Returning a Result from notify would allow error propagation to the caller, which is crucial in a distributed system where failures are common.
The previous comment on this issue is still valid.

src/mito2/src/compaction.rs (1)

108-109: Approve addition of plugins field and async modifications.

The addition of the plugins field to CompactionScheduler and the conversion of methods to async are well-aligned with the system's evolution towards supporting remote compactions and asynchronous operations.

Also applies to: 119-128

src/mito2/src/request.rs (5)

390-390: Well-structured and clear implementation of OutputTx.

The OutputTx struct and its send method are well-implemented, providing a clear and encapsulated way to handle result transmission. The choice to ignore the send result is appropriate here, avoiding unnecessary complexity.

Also applies to: 397-398

---

Line range hint 408-408: Consider simplifying OptionOutputTx by removing the Option wrapper.

The implementation of OptionOutputTx is robust, but as per previous discussions in the PR, consider whether the Option wrapper is necessary. Using a direct OutputTx or an empty vector might simplify the code.
[REFACTOR_SUGGESTion]

Also applies to: 410-410, 412-413, 415-416, 418-419, 421-425

---

Line range hint 427-429: Good use of the OnFailure trait for consistent error handling.

The introduction of the OnFailure trait helps standardize how errors are handled across different parts of the system, which is a good practice for maintainability.

---

Line range hint 431-434: Clear and effective struct definitions for request handling.

SenderWriteRequest and WorkerRequest are well-defined and align well with the system’s architecture for handling different types of requests.

---

Line range hint 436-493: Comprehensive implementation of background job notifications.

The BackgroundNotify enum and related structs like FlushFinished and CompactionFinished are well-implemented, providing detailed handling for various background job results. This enhances the system's responsiveness and reliability.

[coderabbitai]

Actionable comments posted: 0

Outside diff range and nitpick comments (1)

src/mito2/src/compaction.rs (1)

Line range hint 232-354: Evaluate the fallback mechanism for remote compaction.

The logic to fallback to local compaction if remote scheduling fails is crucial for reliability. Review the implementation to ensure that the fallback mechanism is seamless and does not introduce delays or inconsistencies in the compaction process. Consider adding more detailed logging at each step of the fallback to improve observability.

+                debug!("Fallback initiated for region {}", region_id);

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between d299592 and dfb24c3.

Files selected for processing (2)

• src/mito2/src/compaction.rs (16 hunks)
• src/mito2/src/schedule/remote_job_scheduler.rs (1 hunks)

Files skipped from review as they are similar to previous changes (1)

• src/mito2/src/schedule/remote_job_scheduler.rs

Additional comments not posted (7)

src/mito2/src/compaction.rs (7)

29-30: Approved the enhanced logging capability.

The addition of info logging is consistent with the PR's objectives to enhance telemetry within the system.

---

108-109: Approved the addition of the plugins field to CompactionScheduler.

Integrating the Plugins struct is crucial for managing the remote job scheduler functionality, aligning with the PR's objectives to enhance compaction task handling.

---

119-119: Approved the changes to the CompactionScheduler constructor.

Accepting a Plugins parameter and initializing the plugins field are necessary steps to enable the use of the plugin system for remote job scheduling.

Also applies to: 128-128

---

134-134: Approved the asynchronous modification of schedule_compaction.

Converting schedule_compaction to an asynchronous method is essential for non-blocking operations, especially given the potentially long-running nature of compaction tasks.

Also applies to: 162-162

---

166-166: Approved the asynchronous modification of on_compaction_finished.

Making on_compaction_finished asynchronous ensures that the method can handle complex logic and external calls effectively without blocking the system.

Also applies to: 191-191

---

356-363: Approved the changes to remove_region_on_failure.

Enhancing the remove_region_on_failure method to handle region removal on failure improves the robustness and cleanliness of state management within the system.

---

Line range hint 640-762: Approved the addition of test cases for compaction scheduling.

The new test cases effectively verify the behavior of the compaction scheduling logic under various conditions, enhancing the reliability and test coverage of the system.

[coderabbitai]

Actionable comments posted: 0

Outside diff range and nitpick comments (5)

src/mito2/src/compaction.rs (5)

108-109: Review the integration of Plugins into CompactionScheduler.

The integration of Plugins to manage compaction-related plugins is a strategic enhancement. It aligns with the PR's objective to facilitate plugin-based remote job scheduling. Ensure that the Plugins object is properly managed across its lifecycle, particularly regarding ownership and mutability concerns.

Also applies to: 119-128

---

Line range hint 166-191: Assess the error handling in on_compaction_finished.

The method correctly handles the scenario where no compaction tasks are pending for a region. However, the error handling could be more robust by ensuring that all potential error paths are logged and handled appropriately to prevent silent failures.

+            error!("No pending compaction tasks for region {}", region_id);

---

[!TIP]
Codebase Verification

Asynchronous Handling of schedule_compaction Calls Needs Adjustment

The calls to the schedule_compaction function in the following files are not properly handling its asynchronous nature. These calls should use the .await keyword to ensure correct execution:

• src/mito2/src/worker/handle_compaction.rs

  .compaction_scheduler
  .schedule_compaction(

• src/mito2/src/worker/handle_flush.rs

  .compaction_scheduler
  .schedule_compaction(

Please update these calls to handle the asynchronous nature correctly by using .await.

Analysis chain

Line range hint 134-162: Evaluate the asynchronous implementation of schedule_compaction.

Transforming schedule_compaction to an async function supports the non-blocking scheduling of compaction tasks. However, ensure that all calling functions are also adapted to handle this asynchronous nature to prevent runtime issues.

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify all function calls to `schedule_compaction` are properly handling its asynchronous nature.

# Test: Search for the function usage. Expect: Correct async handling in all occurrences.
rg --type rust $'schedule_compaction'

Length of output: 910

---

---

Script:

#!/bin/bash
# Description: Verify all function calls to `schedule_compaction` are properly handling its asynchronous nature.

# Check each call to `schedule_compaction` for proper async handling.
rg --type rust 'schedule_compaction' -A 2

Length of output: 2730

---

---

Script:

#!/bin/bash
# Description: Verify all function calls to `schedule_compaction` are properly handling its asynchronous nature.

# Check the context of calls to `schedule_compaction` in the specified files for proper async handling.
rg 'schedule_compaction' src/mito2/src/worker/handle_compaction.rs -A 5 -B 5
rg 'schedule_compaction' src/mito2/src/worker/handle_flush.rs -A 5 -B 5

Length of output: 883

---

[!TIP]
Codebase Verification

Enhance logging for fallback to local compaction

The fallback to local compaction is currently logged with the statement:

debug!("Remote compaction is not enabled, fallback to local compaction for region {}", region_id);

However, the review comment suggests adding a more explicit log statement to improve traceability. Consider adding the following log statement to ensure comprehensive logging:

debug!("Initiating fallback to local compaction for region {}", region_id);

• File: src/mito2/src/compaction.rs
• Lines: Add the suggested log statement before the existing fallback log statement.

+            debug!("Initiating fallback to local compaction for region {}", region_id);

Analysis chain

Line range hint 232-354: Review the comprehensive implementation of schedule_compaction_request.

This function now handles both local and remote compaction scenarios, including fallback mechanisms. It's crucial to ensure that the fallback to local compaction is seamless and does not introduce additional latency or resource overhead. Also, consider enhancing the logging during the fallback process to improve traceability.

+            debug!("Initiating fallback to local compaction for region {}", region_id);

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify the seamless operation of the fallback mechanism in `schedule_compaction_request`.

# Test: Search for the function usage. Expect: No reported issues or delays in fallback scenarios.
rg --type rust $'schedule_compaction_request'

Length of output: 279

---

---

Script:

#!/bin/bash
# Description: Verify the presence of the debug log statement indicating the fallback to local compaction.

# Search for the debug log statement within the `schedule_compaction_request` function.
rg 'debug!\("Initiating fallback to local compaction for region \{\}", region_id\);' --type rust

Length of output: 98

---

---

Script:

#!/bin/bash
# Description: Verify the presence of debug log statements related to the fallback mechanism in `schedule_compaction_request`.

# Search for all debug log statements within the `schedule_compaction_request` function.
rg 'debug!' --type rust -A 5 -B 5

Length of output: 121233

---

[!TIP]
Codebase Verification

Ensure comprehensive testing of asynchronous compaction scheduling.

The current codebase lacks direct test functions for schedule_compaction. It is crucial to add comprehensive test cases covering various scenarios, including edge cases and error paths, to ensure robust and reliable compaction scheduling.

• Add test cases for schedule_compaction in src/mito2/src/compaction.rs and related files.
• Ensure coverage includes edge cases and fallback mechanisms in remote compaction.

Analysis chain

Line range hint 640-659: Ensure comprehensive testing of asynchronous compaction scheduling.

The tests provided seem to cover various scenarios, including empty and populated compaction tasks. However, ensure that these tests are comprehensive enough to cover all edge cases, especially those involving error paths and fallback mechanisms in remote compaction.

Would you like help in enhancing the test coverage or designing specific test cases for edge scenarios?

Also applies to: 699-728, 740-762

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify the coverage and effectiveness of tests for `schedule_compaction`.

# Test: Search for additional test cases in the test suite. Expect: Comprehensive coverage including edge cases.
rg --type rust $'test_schedule_compaction'

Length of output: 41

---

---

Script:

#!/bin/bash
# Description: Verify the coverage and effectiveness of tests for `schedule_compaction`.

# Search for the `schedule_compaction` function definition to understand its context.
ast-grep --lang rust --pattern 'fn schedule_compaction($$_) { $$$ }'

# Search for test cases that call the `schedule_compaction` function.
rg --type rust 'schedule_compaction'

Length of output: 981

---

---

Script:

#!/bin/bash
# Description: Identify test functions in files where `schedule_compaction` is invoked.

# List the files where `schedule_compaction` is used.
files=$(rg --files-with-matches 'schedule_compaction')

# Search for test functions within these files.
for file in $files; do
  echo "Searching for test functions in $file"
  ast-grep --lang rust --pattern 'fn test_$$_() { $$$ }' $file
done

Length of output: 1276

---

---

Script:

#!/bin/bash
# Description: Identify all test functions across the codebase and check if any involve `schedule_compaction`.

# Search for all test functions across the codebase.
ast-grep --lang rust --pattern 'fn test_$$_() { $$$ }'

# Search for invocations of `schedule_compaction` within the identified test functions.
rg --type rust 'schedule_compaction'

Length of output: 967

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between dfb24c3 and ca1c8fa.

Files selected for processing (3)

• src/mito2/src/compaction.rs (16 hunks)
• src/mito2/src/error.rs (5 hunks)
• src/mito2/src/schedule/remote_job_scheduler.rs (1 hunks)

Additional comments not posted (7)

src/mito2/src/schedule/remote_job_scheduler.rs (3)

62-65: Consider making Notifier.notify method return a Result.

The notify method in the Notifier trait currently does not return a Result, which might limit error handling capabilities. Consider modifying this to return a Result<(), Error> to allow propagation of errors to the caller.
[REFACTOR_Suggestion]

-    async fn notify(&self, result: RemoteJobResult, waiters: Vec<OutputTx>);
+    async fn notify(&self, result: RemoteJobResult, waiters: Vec<OutputTx>) -> Result<(), Error>;

---

130-174: Review error handling strategy in DefaultNotifier.notify.

The method notify in the DefaultNotifier implementation logs errors but does not propagate them. Consider enhancing error propagation and making the error messages more specific to improve system robustness and debuggability.
[REFACTOR_Suggestion]

-                    error!(
-                        "Failed to notify compaction job status for region {}, error: {:?}",
-                        result.region_id, e
-                    );
+                    return Err(Error::from(e));

---

54-58: Ensure Notifier is provided correctly in RemoteJobScheduler.schedule.

The method schedule in the RemoteJobScheduler trait takes a Box<dyn Notifier> as a parameter. Ensure that the lifecycle and ownership of this Notifier are managed properly, especially since it's being passed around in an async environment.

src/mito2/src/error.rs (3)

758-765: Review new RemoteJobScheduler error variant.

The new RemoteJobScheduler error variant includes waiters which is a good design for notifying about fallbacks. Ensure that this pattern is consistently used across other similar error handling scenarios.

---

767-773: Check the implementation of ParseJobId error variant.

The ParseJobId error variant is well-implemented, leveraging the underlying uuid::Error. This is a good practice as it uses existing error handling mechanisms provided by the uuid crate.

---

799-805: Ensure correct usage of waiters_in_remote_job_scheduler method.

The method waiters_in_remote_job_scheduler extracts waiters from the RemoteJobScheduler error variant. Verify that this method is used correctly throughout the codebase to ensure proper error recovery and notification.

Verification successful

---

Correct usage of waiters_in_remote_job_scheduler method confirmed.

The method is used within an error handling block to manage waiters properly when remote compaction scheduling fails, ensuring proper fallback to local compaction.

• src/mito2/src/compaction.rs: Correctly handles errors and uses the method to return waiters for local compaction.

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify the usage of `waiters_in_remote_job_scheduler` method across the codebase.

# Test: Search for usage of the method. Expect: Correct usage patterns.
rg --type rust $'waiters_in_remote_job_scheduler'

Length of output: 234

---

---

Script:

#!/bin/bash
# Description: Extract the context where `waiters_in_remote_job_scheduler` is used in `src/mito2/src/compaction.rs`.

# Search for the method usage and extract surrounding lines for context.
rg --type rust -C 5 'waiters_in_remote_job_scheduler' src/mito2/src/compaction.rs

Length of output: 602

src/mito2/src/compaction.rs (1)

29-30: Approval of new imports for telemetry and plugins.

The addition of common_base::Plugins and telemetry functions (debug, error, info) are appropriate for the new functionalities related to remote job scheduling and enhanced logging capabilities.

[coderabbitai]

Actionable comments posted: 3

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ca1c8fa and a9b2d5e.

Files selected for processing (7)

• src/datanode/src/datanode.rs (5 hunks)
• src/mito2/src/compaction.rs (16 hunks)
• src/mito2/src/compaction/compactor.rs (2 hunks)
• src/mito2/src/engine.rs (3 hunks)
• src/mito2/src/region/options.rs (5 hunks)
• src/mito2/src/test_util.rs (8 hunks)
• src/store-api/src/mito_engine_options.rs (1 hunks)

Additional comments not posted (12)

src/store-api/src/mito_engine_options.rs (1)

28-28: Ensure the correctness of the added key.

The key "compaction.twcs.remote_compaction" has been added to the list of valid keys. Ensure that this key is correctly implemented and utilized in the codebase.

Verification successful

---

The key "compaction.twcs.remote_compaction" is correctly added to the list of valid keys and has a default value in the options. No further issues found.

• src/store-api/src/mito_engine_options.rs: The key is part of a list of valid keys.
• src/mito2/src/region/options.rs: The key is paired with a default value of "false".

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify the usage of the new key in the codebase.

# Test: Search for the key usage. Expect: Relevant usage in the codebase.
rg --type rust 'compaction.twcs.remote_compaction'

Length of output: 228

---

---

Script:

#!/bin/bash
# Description: Inspect the usage of the key in the identified files.

# Check the usage in src/store-api/src/mito_engine_options.rs
echo "Inspecting src/store-api/src/mito_engine_options.rs"
rg --context 5 'compaction.twcs.remote_compaction' src/store-api/src/mito_engine_options.rs

# Check the usage in src/mito2/src/region/options.rs
echo "Inspecting src/mito2/src/region/options.rs"
rg --context 5 'compaction.twcs.remote_compaction' src/mito2/src/region/options.rs

Length of output: 1293

---

---

Script:

#!/bin/bash
# Description: Conduct a comprehensive search for the key usage across the codebase.

# Search for the key usage in the codebase
rg 'compaction.twcs.remote_compaction'

Length of output: 216

src/mito2/src/compaction/compactor.rs (1)

20-20: Ensure the correctness of the updated function signature.

The function signature of open_compaction_region has been updated to use ObjectStoreManagerRef. Ensure that this change is correctly implemented and utilized in the codebase.

Verification successful

To ensure the correctness of the updated function signature and its implementation, we need to verify the usage of ObjectStoreManagerRef within the open_compaction_region function.

---

To verify the correct usage of ObjectStoreManagerRef within the open_compaction_region function, let's list all functions in the compactor.rs file and inspect their implementations.

---

To ensure thorough verification, let's search for the usage of ObjectStoreManagerRef directly within the compactor.rs file.

---

To ensure the correct implementation of the object_store_manager parameter, let's search for its usage within the compactor.rs file.

---

To confirm the correctness of the implementation, let's extract the lines around the usage of object_store_manager.default_object_store() to understand its context.

---

The function signature change to use ObjectStoreManagerRef is correctly implemented and utilized in the codebase.

• The object_store_manager parameter is correctly used within the function to fetch the default object store when necessary.

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify the usage of the updated function signature in the codebase.

# Test: Search for the function usage. Expect: Relevant usage in the codebase.
rg --type rust 'open_compaction_region'

Length of output: 114

---

---

Script:

#!/bin/bash
# Description: Verify the usage of ObjectStoreManagerRef within the open_compaction_region function.

# Search for the implementation of open_compaction_region and its usage of ObjectStoreManagerRef
ast-grep --lang rust --pattern $'pub async fn open_compaction_region($$_: ObjectStoreManagerRef, $$$) {
  $$$
}' 

Length of output: 113

---

---

Script:

#!/bin/bash
# Description: List all functions in the compactor.rs file and check their implementations.

# List all function definitions in the compactor.rs file
ast-grep --lang rust --pattern 'fn $_($$$) { $$$ }' src/mito2/src/compaction/compactor.rs

Length of output: 91

---

---

Script:

#!/bin/bash
# Description: Search for the usage of ObjectStoreManagerRef within the compactor.rs file.

# Search for ObjectStoreManagerRef within the compactor.rs file
rg 'ObjectStoreManagerRef' src/mito2/src/compaction/compactor.rs

Length of output: 163

---

---

Script:

#!/bin/bash
# Description: Search for the usage of object_store_manager within the compactor.rs file.

# Search for object_store_manager within the compactor.rs file
rg 'object_store_manager' src/mito2/src/compaction/compactor.rs

Length of output: 201

---

---

Script:

#!/bin/bash
# Description: Extract lines around the usage of object_store_manager.default_object_store() to understand its context.

# Extract lines around the usage of object_store_manager.default_object_store()
rg -A 5 -B 5 'object_store_manager.default_object_store()' src/mito2/src/compaction/compactor.rs

Length of output: 401

src/datanode/src/datanode.rs (3)

396-396: Ensure the correctness of the updated function signature.

The function signature of build_mito_engine has been updated to accept plugins: Plugins. Ensure that this change is correctly implemented and utilized in the codebase.

Verification successful

---

The updated function signature of build_mito_engine is correctly implemented and utilized.

• The plugins: Plugins parameter is correctly passed in the function call.
• Within the function, plugins is used as expected in the creation of the MitoEngine.

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify the usage of the updated function signature in the codebase.

# Test: Search for the function usage. Expect: Relevant usage in the codebase.
rg --type rust 'build_mito_engine'

Length of output: 187

---

---

Script:

#!/bin/bash
# Description: Verify the usage and correctness of the updated function signature.

# Test 1: Extract the function call to `build_mito_engine` to check if `plugins: Plugins` is passed correctly.
rg --type rust 'Self::build_mito_engine' -A 5

# Test 2: Extract the implementation of `build_mito_engine` to ensure `plugins: Plugins` is used correctly.
ast-grep --lang rust --pattern $'async fn build_mito_engine($_) {
  $$$
}'

Length of output: 545

---

---

Script:

#!/bin/bash
# Description: Verify the implementation of `build_mito_engine` to ensure `plugins: Plugins` is used correctly.

# Extract the implementation of `build_mito_engine` to verify the usage of `plugins: Plugins`.
rg --type rust 'async fn build_mito_engine' -A 20

Length of output: 1519

---

346-347: Ensure the correctness of the updated function signature.

The function signature of new_region_server has been updated to accept plugins: Plugins. Ensure that this change is correctly implemented and utilized in the codebase.

Verification successful

---

Ensure the correctness of the updated function signature.

The function signature of new_region_server has been correctly updated to utilize self.plugins.clone() within the function body. No issues found with this implementation.

• The function definition includes self.plugins.clone(), ensuring the plugins parameter is correctly utilized.

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify the usage of the updated function signature in the codebase.

# Test: Search for the function usage. Expect: Relevant usage in the codebase.
rg --type rust 'new_region_server'

Length of output: 558

---

---

Script:

#!/bin/bash
# Description: Extract the function definition and call sites for `new_region_server` to verify the updated function signature.

# Extract the function definition
ast-grep --lang rust --pattern 'async fn new_region_server($_)' 

# Extract the call sites
rg --type rust 'self.new_region_server'

Length of output: 216

---

---

Script:

#!/bin/bash
# Description: Extract the full definition of the `new_region_server` function to verify the updated function signature and its usage.

# Extract the full function definition
ast-grep --lang rust --pattern 'async fn new_region_server($_) { $$$ }' 

Length of output: 73

---

---

Script:

#!/bin/bash
# Description: Search for the `new_region_server` function definition and display surrounding lines for context.

# Search for the function definition and display 20 lines of context before and after the match
rg 'async fn new_region_server' -A 20 -B 20

Length of output: 2548

---

361-361: Ensure the correctness of the updated function signature.

The function signature of build_store_engines has been updated to accept plugins: Plugins. Ensure that this change is correctly implemented and utilized in the codebase.

Verification successful

---

The updated function signature is correctly implemented and utilized.

The plugins parameter in the build_store_engines function is appropriately passed and used within the build_mito_engine function, ensuring the correctness of the update.

• build_store_engines calls build_mito_engine with the plugins parameter.
• build_mito_engine uses the plugins parameter directly in creating a new MitoEngine.

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify the usage of the updated function signature in the codebase.

# Test: Search for the function usage. Expect: Relevant usage in the codebase.
rg --type rust 'build_store_engines'

Length of output: 224

---

---

Script:

#!/bin/bash
# Description: Extract the implementation of the `build_store_engines` function to verify the usage of `plugins`.

# Extract the function implementation
ast-grep --lang rust --pattern 'async fn build_store_engines($_, $_, plugins: Plugins) { $$$ }' 

Length of output: 97

---

---

Script:

#!/bin/bash
# Description: Search for the function definition of `build_store_engines` and extract the relevant lines for inspection.

# Search for the function definition
rg 'async fn build_store_engines' --context 20

Length of output: 2705

---

---

Script:

#!/bin/bash
# Description: Extract the implementation of the `build_mito_engine` function to verify the usage of `plugins`.

# Search for the function definition
rg 'async fn build_mito_engine' --context 20

Length of output: 2846

src/mito2/src/engine.rs (1)

280-280: Ensure the correctness of the updated function signature.

The function signature of new in the EngineInner struct has been updated to accept plugins: Plugins. Ensure that this change is correctly implemented and utilized in the codebase.

src/mito2/src/compaction.rs (5)

30-31: LGTM! Necessary imports for new functionality.

The new imports are necessary for the new functionality related to remote job scheduling.

Also applies to: 63-65

---

121-121: LGTM! Constructor updated to accept plugins.

The constructor is correctly updated to accept and initialize the new plugins field.

Also applies to: 130-130

---

Line range hint 136-164: LGTM! Method updated to be asynchronous.

The schedule_compaction method is correctly updated to be asynchronous to support the new functionality.

---

Line range hint 168-193: LGTM! Method updated to be asynchronous.

The on_compaction_finished method is correctly updated to be asynchronous to support the new functionality.

---

110-111: Ensure the new plugins field is properly used and initialized.

The field is correctly added to store plugins for the compaction scheduler. Ensure that this field is properly used and initialized in the code.

Verification successful

Ensure the new plugins field is properly used and initialized.

The field is correctly added to store plugins for the compaction scheduler. Ensure that this field is properly used and initialized in the code.

---

Ensure the new plugins field is properly initialized in the CompactionScheduler.

The field is correctly added to store plugins for the compaction scheduler. Ensure that this field is properly initialized in the code.

---

Ensure the new plugins field is properly initialized in the CompactionScheduler.

The field is correctly added to store plugins for the compaction scheduler. Ensure that this field is properly initialized in the code.

---

The new plugins field is properly used and initialized.

• The plugins field is correctly added to the CompactionScheduler.
• It is properly initialized in the CompactionScheduler::new constructor.

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify if the `plugins` field is properly used and initialized.

# Test: Search for the field usage in the codebase. Expect: Proper initialization and usage.
rg --type rust 'plugins'

Length of output: 16234

---

---

Script:

#!/bin/bash
# Description: Verify if the `plugins` field is properly used and initialized in the `src/mito2/src/compaction.rs` file.

# Test: Search for the initialization and usage of the `plugins` field in the `src/mito2/src/compaction.rs` file.
rg 'plugins' src/mito2/src/compaction.rs

Length of output: 204

---

---

Script:

#!/bin/bash
# Description: Verify the initialization of the `plugins` field in the `src/mito2/src/compaction.rs` file.

# Test: Search for the `CompactionScheduler` struct and its constructors to check the initialization of the `plugins` field.
rg 'CompactionScheduler' src/mito2/src/compaction.rs

Length of output: 155

---

---

Script:

#!/bin/bash
# Description: Verify the initialization of the `plugins` field in the `CompactionScheduler` implementation.

# Test: Search for the initialization of the `plugins` field within the `CompactionScheduler` implementation.
rg 'plugins' -A 10 src/mito2/src/compaction.rs

Length of output: 1374

src/mito2/src/test_util.rs (1)

35-35: Import statement for Plugins added

The Plugins module from common_base is imported. Ensure that this import is necessary and used correctly within the file.

Verification successful

---

Import statement for Plugins added

The Plugins module from common_base is imported and used multiple times within the file src/mito2/src/test_util.rs. The import is necessary and appears to be used correctly.

• Lines where Plugins::new() is used:
  • Line 37
  • Line 38
  • Line 39
  • Line 40
  • Line 41
  • Line 42
  • Line 43
  • Line 44
  • Line 45
  • Line 46

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify the usage of `Plugins` in the file to ensure the import is necessary and used correctly.

# Test: Search for the usage of `Plugins` in the file.
rg --type rust 'Plugins' src/mito2/src/test_util.rs

Length of output: 397

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a9b2d5e and 6502caa.

Files selected for processing (3)

• src/mito2/src/compaction.rs (16 hunks)
• src/mito2/src/region/options.rs (7 hunks)
• src/mito2/src/schedule/remote_job_scheduler.rs (1 hunks)

Files not summarized due to errors (2)

• src/mito2/src/compaction.rs: Error: Server error. Please try again later.
• src/mito2/src/schedule/remote_job_scheduler.rs: Error: Server error. Please try again later.

Files not reviewed due to errors (3)

• src/mito2/src/schedule/remote_job_scheduler.rs (Error: Server error. Please try again later.)
• src/mito2/src/region/options.rs (Error: Server error. Please try again later.)
• src/mito2/src/compaction.rs (Error: Server error. Please try again later.)

[coderabbitai]

Actionable comments posted: 6

Outside diff range and nitpick comments (4)

src/mito2/src/compaction.rs (4)

Line range hint 136-164: Evaluate the fallback mechanism for remote compaction.

The logic to fallback to local compaction if remote scheduling fails is crucial for reliability. Review the implementation to ensure that the fallback mechanism is seamless and does not introduce delays or inconsistencies in the compaction process. Consider adding more detailed logging at each step of the fallback to improve observability.

+                debug!("Fallback initiated for region {}", region_id);

---

Line range hint 168-193: Evaluate the scheduling logic in on_compaction_finished.

The function ensures that the region is always compacted until the picker returns None. Review the implementation to ensure that the scheduling logic is efficient and does not introduce unnecessary delays or inconsistencies.

Would you like me to draft additional documentation detailing the scheduling logic?

---

Line range hint 234-356: Evaluate the fallback mechanism for remote compaction.

The logic to fallback to local compaction if remote scheduling fails is crucial for reliability. Review the implementation to ensure that the fallback mechanism is seamless and does not introduce delays or inconsistencies in the compaction process. Consider adding more detailed logging at each step of the fallback to improve observability.

+                debug!("Fallback initiated for region {}", region_id);

---

Line range hint 110-130: Ensure proper plugin management in CompactionScheduler.

The CompactionScheduler struct now includes a plugins field for managing compaction-related plugins. Ensure that the plugins are correctly initialized and managed throughout the compaction process.

Would you like me to draft additional documentation detailing the plugin management?

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a9b2d5e and 6502caa.

Files selected for processing (3)

• src/mito2/src/compaction.rs (16 hunks)
• src/mito2/src/region/options.rs (7 hunks)
• src/mito2/src/schedule/remote_job_scheduler.rs (1 hunks)

Files not summarized due to errors (2)

• src/mito2/src/schedule/remote_job_scheduler.rs: Error: Server error. Please try again later.
• src/mito2/src/region/options.rs: Error: Server error. Please try again later.

Additional comments not posted (3)

src/mito2/src/region/options.rs (3)

166-170: Ensure the new remote_compaction method is covered by tests.

The method correctly retrieves the remote_compaction flag from TwcsOptions. However, it appears that this method is not currently covered by any unit tests. Please add tests to verify the behavior of the remote_compaction method.

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_remote_compaction() {
        let options = CompactionOptions::Twcs(TwcsOptions {
            remote_compaction: true,
            ..Default::default()
        });
        assert!(options.remote_compaction());

        let options = CompactionOptions::Twcs(TwcsOptions {
            remote_compaction: false,
            ..Default::default()
        });
        assert!(!options.remote_compaction());
    }
}

---

193-195: Ensure the new remote_compaction field is covered by tests.

The remote_compaction field in TwcsOptions is not currently covered by any tests. Please add unit tests to verify its behavior, including serialization and deserialization.

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_twcs_options_serde() {
        let options = TwcsOptions {
            remote_compaction: true,
            ..Default::default()
        };
        let json = serde_json::to_string(&options).unwrap();
        let deserialized: TwcsOptions = serde_json::from_str(&json).unwrap();
        assert!(deserialized.remote_compaction);

        let options = TwcsOptions {
            remote_compaction: false,
            ..Default::default()
        };
        let json = serde_json::to_string(&options).unwrap();
        let deserialized: TwcsOptions = serde_json::from_str(&json).unwrap();
        assert!(!deserialized.remote_compaction);
    }
}

---

220-220: Ensure the default value of remote_compaction is covered by tests.

The Default implementation for TwcsOptions sets remote_compaction to false. Please add tests to verify this behavior.

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_twcs_options_default() {
        let options = TwcsOptions::default();
        assert!(!options.remote_compaction);
    }
}

[evenyag]

@coderabbitai pause

[coderabbitai]

Actions performed

Reviews paused.

[fengjiachun]

LGTM

[fengjiachun]

Suggested change

	// Create a local compaction task.
	// Creates a local compaction task.