Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat: introduce the interface of RemoteJobScheduler #4124

Conversation

zyy17
Copy link
Collaborator

@zyy17 zyy17 commented Jun 9, 2024

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.

@github-actions github-actions bot added the docs-not-required This change does not impact docs. label Jun 9, 2024
@zyy17 zyy17 force-pushed the feat/add-experimental-remote-job-scheduler branch from c9d3214 to 90a4794 Compare June 17, 2024 03:28
@zyy17 zyy17 marked this pull request as ready for review June 19, 2024 10:07
@zyy17 zyy17 requested review from v0y4g3r, evenyag, waynexia and a team as code owners June 19, 2024 10:07
@zyy17 zyy17 requested review from sunng87 and shuiyisong June 19, 2024 10:07
@zyy17
Copy link
Collaborator Author

zyy17 commented Jun 19, 2024

@sunng87 @shuiyisong The PR contains part of the plugins of datanode, PTAL.

@zyy17
Copy link
Collaborator Author

zyy17 commented Jun 19, 2024

@evenyag @v0y4g3r When I tried to design the API for rebuilding the job context if the datanode is restarted, I realized that we might need to keep metadata about the latest compaction status. It's useful if we use local compaction for heavy compaction tasks.

The metadata may be part of region metadata, for example:

last_compaction: {
    "timestamp": xxx,
    "status": "Processing"
}

When starting the datanode, it can fetch the metadata(open regions) and decide whether to schedule the compaction task.

Copy link

codecov bot commented Jun 19, 2024

Codecov Report

Attention: Patch coverage is 56.11814% with 104 lines in your changes missing coverage. Please review.

Project coverage is 84.83%. Comparing base (22d1268) to head (d08b56e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4124      +/-   ##
==========================================
- Coverage   85.16%   84.83%   -0.33%     
==========================================
  Files        1020     1021       +1     
  Lines      179630   179777     +147     
==========================================
- Hits       152976   152519     -457     
- Misses      26654    27258     +604     


/// RemoteJobScheduler is a trait that defines the API to schedule remote jobs.
#[async_trait::async_trait]
pub trait RemoteJobScheduler: Send + Sync + 'static {
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a single trait for both remote and local job?

Copy link
Collaborator Author

@zyy17 zyy17 Jun 20, 2024

Choose a reason for hiding this comment

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

I think it's possible in theory, for example:

#[async_trait::async_trait]
pub trait Scheduler: Send + Sync + 'static {
  async fn schedule(&self, job: Job, notifier: Option<Arc<dyn Notifier>>) -> Result<Option<JobId>>;
}

@@ -693,7 +693,7 @@ pub(crate) struct CompactionFinished {
/// Region id.
pub(crate) region_id: RegionId,
/// Compaction result senders.
pub(crate) senders: Vec<OutputTx>,
pub(crate) senders: Option<Vec<OutputTx>>,
Copy link
Member

Choose a reason for hiding this comment

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

I remember we use empty vec for None in our code base @evenyag

@zyy17 zyy17 closed this Jun 20, 2024
@zyy17 zyy17 force-pushed the feat/add-experimental-remote-job-scheduler branch from d08b56e to 5bcd7a1 Compare June 20, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required This change does not impact docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants