feat(meta): add CompactorManager #556

zwang28 · 2022-02-25T07:25:38Z

What's changed and what's your intention?

This PR adds CompactorManager in meta node.

It maintains available compactors running in compute nodes.
It dispatches compact task to compactors via server-streaming RPC, in a round-robin approach.

Detailed in #546.

After this PR is merged, there will be no compaction at all temporarily, until compactor-node is integrated in next PR.

Checklist

I have written necessary docs and comments
I have added necessary unit tests

Refer to a related PR or issue link (optional)

#546

codecov · 2022-02-25T08:03:24Z

Codecov Report

Merging #556 (c56830e) into main (15da24f) will increase coverage by 0.01%.
The diff coverage is 69.44%.

@@             Coverage Diff              @@
##               main     #556      +/-   ##
============================================
+ Coverage     71.35%   71.37%   +0.01%     
  Complexity     2683     2683              
============================================
  Files           883      886       +3     
  Lines         50561    50637      +76     
  Branches       1723     1723              
============================================
+ Hits          36079    36142      +63     
- Misses        13678    13691      +13     
  Partials        804      804

Flag	Coverage Δ
java	`59.29% <ø> (ø)`
rust	`76.63% <69.44%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
rust/meta/src/hummock/compaction.rs	`67.97% <0.00%> (-0.91%)`	⬇️
rust/meta/src/hummock/mock_hummock_meta_client.rs	`54.83% <0.00%> (-1.42%)`	⬇️
rust/meta/src/rpc/service/mod.rs	`0.00% <0.00%> (ø)`
rust/storage/src/hummock/hummock_meta_client.rs	`0.00% <0.00%> (ø)`
rust/storage/src/hummock/shared_buffer.rs	`87.02% <ø> (-0.16%)`	⬇️
rust/meta/src/hummock/mod.rs	`14.81% <14.81%> (ø)`
rust/meta/src/rpc/service/hummock_service.rs	`1.12% <16.66%> (-0.02%)`	⬇️
rust/meta/src/hummock/hummock_manager.rs	`68.94% <40.00%> (+0.34%)`	⬆️
rust/meta/src/hummock/compactor_manager.rs	`97.64% <97.64%> (ø)`
rust/meta/src/hummock/hummock_manager_tests.rs	`91.70% <100.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15da24f...c56830e. Read the comment docs.

soundOfDestiny

seems ok in first glance

fuyufjh · 2022-02-28T07:22:20Z

rust/meta/src/hummock/compactor_manager.rs

+const COMPACT_TASK_TIMEOUT: Duration = Duration::from_secs(60);
+
+/// `CompactorManager` maintains compactors which can process compact task.
+pub struct CompactorManager<E> {


There is a RwLock or Atomic for each field, which may cause non-atomic behaviors.

I think it's okay to simply create a struct CompactorManagerCore (or whatever) and protect it with a RwLock/Mutex<CompactorManagerCore>, which could make multi-threading easier, especially if more fields added in the future.

fuyufjh · 2022-02-28T07:24:04Z

rust/meta/src/rpc/service/hummock_service.rs

 use crate::storage::MetaStore;

 pub struct HummockServiceImpl<S>
 where
 S: MetaStore,
 {
 hummock_manager: Arc<HummockManager<S>>,
+ compaction_manager: Arc<CompactorManager<tonic::Status>>,


Suggested change

compaction_manager: Arc<CompactorManager<tonic::Status>>,

compactor_manager: Arc<CompactorManager<tonic::Status>>,

Thanks. FIxed.

fuyufjh · 2022-02-28T07:25:52Z

rust/meta/src/hummock/compactor_manager.rs

+/// `CompactorManager` maintains compactors which can process compact task.
+pub struct CompactorManager<E> {
+ /// Stores senders of stream to available compactors
+ compactors: RwLock<Vec<Sender<std::result::Result<CompactTasksResponse, E>>>>,


Is it necessary to use generic (E) here?

Tried to not introduce tonic stuffs to CompactorManager.
But another NotificationManager actually uses tonic::Status already. So yes E may be removed to keep consistency.

recommend to avoid generics unless inevitable

Simply use RwError here, we can later convert it to tonic Status

Fixed. Add a wrapper struct RwReceiverStream to map Result<S, RwError> to Result<S, tonic::Status>.

fuyufjh · 2022-02-28T07:29:44Z

rust/meta/src/rpc/service/hummock_service.rs

+
+ type CompactTasksStream = ReceiverStream<Result<CompactTasksResponse, Status>>;
+
+ async fn compact_tasks(


get_compact_tasks ?

I would suggest to apply this renaming in next PR. Currently there is already a RPC named GetCompactionTasks, which is used by compactors to poll tasks. After we refactor the compactors in next PR, the legacy GetCompactionTasks can be removed.

soundOfDestiny · 2022-02-28T09:22:27Z

rust/meta/src/hummock/mod.rs

+ }
+ }
+ Err(e) => {
+ error!("failed to try_assign_compact_task {}", e);


(1) seems try_assign_compact_task will not return Err
(2) if try_assign_compact_task returns Err in the future, shall we call report_compact_task to report a compact failure here?

You are right. try_assign_compact_task can simply return bool. And report_compact_task to cancel the task if false is returned.

You are right. try_assign_compact_task can simply return bool. And report_compact_task to cancel the task if false is returned.

ok so we only need to remove this matching arm

MrCroxx · 2022-02-28T10:10:15Z

rust/meta/src/hummock/mod.rs

+
+const COMPACT_TRIGGER_INTERVAL: Duration = Duration::from_secs(10);
+/// Starts a worker to conditionally trigger compaction.
+pub async fn start_compaction_loop<S, E>(


Are all SSTable being compacted pinned? If not, should we pin them first?

If a SST marked deletion by compation is being referenced by any pinned version, it won't be deleted at that time.
We don’t need to explictly pin when compaction, because the vacuum(WIP) knows when it's safe to delete a SST.

Remove unnecessary Result wrapper in try_assign_compact_task. Add CompactorManagerInner.

soundOfDestiny · 2022-03-01T08:57:51Z

rust/meta/src/hummock/mod.rs

+ };
+ if !compactor_manager_ref
+ .try_assign_compact_task(compact_task.clone())
+ .await


latest change here seems that it has satisfied my need

# Conflicts: # rust/storage/src/hummock/hummock_meta_client.rs # rust/storage/src/hummock/mod.rs

zwang28 force-pushed the wangzheng/refactor_compaction branch 2 times, most recently from 8c1134b to c15ff67 Compare February 25, 2022 07:50

feat: add CompactorManager

94793d9

zwang28 force-pushed the wangzheng/refactor_compaction branch from c15ff67 to 94793d9 Compare February 25, 2022 08:08

github-actions bot added the type/feature label Feb 25, 2022

zwang28 marked this pull request as ready for review February 25, 2022 08:23

zwang28 requested review from soundOfDestiny, skyzh and twocode February 25, 2022 08:25

soundOfDestiny reviewed Feb 26, 2022

View reviewed changes

fuyufjh reviewed Feb 28, 2022

View reviewed changes

soundOfDestiny reviewed Feb 28, 2022

View reviewed changes

MrCroxx reviewed Feb 28, 2022

View reviewed changes

zwang28 and others added 2 commits February 28, 2022 22:39

Remove unnecessary generic in CompactorManager.

23efaac

Remove unnecessary Result wrapper in try_assign_compact_task. Add CompactorManagerInner.

Merge branch 'main' into wangzheng/refactor_compaction

5608097

soundOfDestiny approved these changes Mar 1, 2022

View reviewed changes

zwang28 added 2 commits March 2, 2022 14:00

Merge branch 'main' into wangzheng/refactor_compaction

a5c7fa7

# Conflicts: # rust/storage/src/hummock/hummock_meta_client.rs # rust/storage/src/hummock/mod.rs

fix tests

c56830e

zwang28 merged commit 7af70bc into main Mar 2, 2022

zwang28 deleted the wangzheng/refactor_compaction branch March 2, 2022 06:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(meta): add CompactorManager #556

feat(meta): add CompactorManager #556

zwang28 commented Feb 25, 2022 •

edited

Loading

codecov bot commented Feb 25, 2022 •

edited

Loading

soundOfDestiny left a comment

fuyufjh Feb 28, 2022

zwang28 Feb 28, 2022

fuyufjh Feb 28, 2022

zwang28 Feb 28, 2022

fuyufjh Feb 28, 2022

zwang28 Feb 28, 2022

fuyufjh Feb 28, 2022

skyzh Feb 28, 2022

zwang28 Feb 28, 2022

fuyufjh Feb 28, 2022

zwang28 Feb 28, 2022

soundOfDestiny Feb 28, 2022

zwang28 Feb 28, 2022 •

edited

Loading

soundOfDestiny Feb 28, 2022 •

edited

Loading

zwang28 Feb 28, 2022

MrCroxx Feb 28, 2022

zwang28 Feb 28, 2022 •

edited

Loading

soundOfDestiny Mar 1, 2022

	compaction_manager: Arc<CompactorManager<tonic::Status>>,
	compactor_manager: Arc<CompactorManager<tonic::Status>>,


		type CompactTasksStream = ReceiverStream<Result<CompactTasksResponse, Status>>;

		async fn compact_tasks(

feat(meta): add CompactorManager #556

feat(meta): add CompactorManager #556

Conversation

zwang28 commented Feb 25, 2022 • edited Loading

What's changed and what's your intention?

Checklist

Refer to a related PR or issue link (optional)

codecov bot commented Feb 25, 2022 • edited Loading

Codecov Report

soundOfDestiny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwang28 Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

soundOfDestiny Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwang28 Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwang28 commented Feb 25, 2022 •

edited

Loading

codecov bot commented Feb 25, 2022 •

edited

Loading

zwang28 Feb 28, 2022 •

edited

Loading

soundOfDestiny Feb 28, 2022 •

edited

Loading

zwang28 Feb 28, 2022 •

edited

Loading