feat(storage): seperate timeout between process and heartbeat #14366

Li0k · 2024-01-04T10:20:57Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

#14327

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

Little-Wallace · 2024-01-05T03:37:49Z

src/meta/src/hummock/compactor_manager.rs

    }

    fn get_heartbeat_expired_tasks(
        task_heartbeats: &HashMap<HummockCompactionTaskId, TaskHeartbeat>,
-        now: u64,
+        heartbeat_expiry_ts: u64,


is it a typo ?

It's not a typo. After we distinguish process and heartbeat timeout, the background thread only needs to check expired_task based on heartbeat_expiry_ts

Little-Wallace · 2024-01-05T04:46:43Z

src/meta/src/hummock/compactor_manager.rs

@@ -321,15 +332,19 @@ impl CompactorManagerInner {
        let mut cancel_tasks = vec![];
        for progress in progress_list {
            if let Some(task_ref) = self.task_heartbeats.get_mut(&progress.task_id) {
+                task_ref.update_at = now;


In original implement, we update "expire_at" to "expire_at + 60 secs". And we check it with "now - 30secs". It means that the task will be cancled after "90s" not changed.
But now, we will cancel task if it did not receive heartbeat during "30s". Is it too short ?

src/meta/src/hummock/compactor_manager.rs

zwang28

LGTM

src/meta/src/hummock/compactor_manager.rs

Little-Wallace

LGTM

wenym1

Rest LGTM!

wenym1 · 2024-01-05T09:26:38Z

src/storage/src/hummock/compactor/task_progress.rs

-        self.num_progress_key
-            .fetch_add(inc_key_num, Ordering::Relaxed);
+    pub fn inc_num_pending_read_io(&self) {
+        self.num_pending_read_io


Any reason to use SeqCst? If this atomic operation is frequent, there may be some performance issue.

This operation is infrequent, but it does not require precise values. I will change it to Relaxed.

wenym1 · 2024-01-05T09:28:50Z

src/meta/src/hummock/compactor_manager.rs

            .duration_since(SystemTime::UNIX_EPOCH)
            .expect("Clock may have gone backwards")
            .as_secs()
-            - interval;
-        Self::get_heartbeat_expired_tasks(&self.task_heartbeats, now)
+            - self.heartbeat_expiry_seconds;


expiry -> expire?

Signed-off-by: Bugen Zhao <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Co-authored-by: August <[email protected]>

…behavior (#14345) Signed-off-by: TennyZhuang <[email protected]>

Co-authored-by: Bohan Zhang <[email protected]>

Signed-off-by: tabVersion <[email protected]> Co-authored-by: cheskel <[email protected]> Co-authored-by: xxchan <[email protected]>

…es (#14364) Signed-off-by: TennyZhuang <[email protected]>

…ave been marked as created (#14367) Co-authored-by: zwang28 <[email protected]>

Co-authored-by: KeXiangWang <[email protected]>

…nto li0k/storage_task_expire

Li0k · 2024-01-10T12:02:30Z

@hzxa21 @wenym1 PTAL

…nto li0k/storage_task_expire

hzxa21 · 2024-01-09T04:16:59Z

src/meta/src/hummock/compactor_manager.rs

            .duration_since(SystemTime::UNIX_EPOCH)
            .expect("Clock may have gone backwards")
            .as_secs()
-            - interval;
-        Self::get_heartbeat_expired_tasks(&self.task_heartbeats, now)
+            - self.heartbeat_expiry_seconds;


hzxa21 · 2024-01-15T06:42:02Z

src/storage/src/hummock/iterator/merge_inner.rs

@@ -104,6 +108,11 @@ pub struct MergeIteratorInner<I: HummockIterator, NE: NodeExtraOrderInfo> {
    heap: BinaryHeap<Node<I, NE>>,

    last_table_key: Vec<u8>,
+
+    // compactor task progress report
+    task_progress: Option<Arc<TaskProgress>>,


I think it is a bad idea to embed a compactor related concept into a generic merge iterator. cc @wenym1

Also, compact_and_build_sst takes a HummockIterator, not MergeIterator so it is still possible we miss to update task progress. How about having a wrapper iterator that impl HummockIterator to wrap the underlying HummockIterator to do key progress tracking.

Thanks, introduced a new wrapper iterator to check the processed key

…nto li0k/storage_task_expire

wenym1

LGTM!

Signed-off-by: Bugen Zhao <[email protected]> Signed-off-by: TennyZhuang <[email protected]> Signed-off-by: tabVersion <[email protected]> Co-authored-by: Bugen Zhao <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Eric Fu <[email protected]> Co-authored-by: August <[email protected]> Co-authored-by: TennyZhuang <[email protected]> Co-authored-by: xxchan <[email protected]> Co-authored-by: Bohan Zhang <[email protected]> Co-authored-by: cheskel <[email protected]> Co-authored-by: Xinhao Xu <[email protected]> Co-authored-by: Yuhao Su <[email protected]> Co-authored-by: stonepage <[email protected]> Co-authored-by: zwang28 <[email protected]> Co-authored-by: Kexiang Wang <[email protected]> Co-authored-by: KeXiangWang <[email protected]>

…#14366)" This reverts commit c9e3e92.

feat(storage): seperate timeout between process and heartbeat

635f777

Li0k requested review from hzxa21, Little-Wallace, wenym1 and zwang28 January 4, 2024 10:20

github-actions bot added the type/feature label Jan 4, 2024

Little-Wallace reviewed Jan 5, 2024

View reviewed changes

src/meta/src/hummock/compactor_manager.rs Outdated Show resolved Hide resolved

zwang28 approved these changes Jan 5, 2024

View reviewed changes

src/meta/src/hummock/compactor_manager.rs Outdated Show resolved Hide resolved

Little-Wallace approved these changes Jan 5, 2024

View reviewed changes

wenym1 approved these changes Jan 5, 2024

View reviewed changes

hzxa21 mentioned this pull request Jan 9, 2024

chore: lower object store IO timeout #14428

Closed

9 tasks

BugenZhao and others added 17 commits January 10, 2024 18:54

fix(dashboard): minor style tweaks (#14356)

974fa53

Signed-off-by: Bugen Zhao <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

fix: ignore dependent table_fragment not exist (#14308)

191ba77

Co-authored-by: August <[email protected]>

fix(stream/materialize): incorrect implementation in ignore-conflict …

1aff233

…behavior (#14345) Signed-off-by: TennyZhuang <[email protected]>

refactor(frontend): simplify bind_columns_from_source (#14335)

0217506

Co-authored-by: Bohan Zhang <[email protected]>

fix: pubsub auth (#14360)

f3eafad

Signed-off-by: tabVersion <[email protected]> Co-authored-by: cheskel <[email protected]> Co-authored-by: xxchan <[email protected]>

feat(sink): support es sink struct and refactor es sink (#14231)

5bf6c46

feat(catalog): add pg_get_viewdef (#14336)

63103d4

chore(storage): update config

e038eb1

fix(storage):fix ut

fcc827b

chore(storage): rename

8bc1996

fix(stream/materialize): enforce update_cache when modify fixed_chang…

c0409ca

…es (#14364) Signed-off-by: TennyZhuang <[email protected]>

test: reorganize the sink to table e2e test (#14158)

be1c706

test: fix flaky sink_into_table test (#14372)

d44270e

test: bump sqllogictest & fix backwards-compat-test/e2e-tests (#14354)

abee759

fix: Do not recover background streaming jobs whose table fragments h…

7a784cc

…ave been marked as created (#14367) Co-authored-by: zwang28 <[email protected]>

chore(storage): ctl support cancel specific task (#14325)

ca74b3c

feat(connector): support local fs source (#14312)

1795e6c

Co-authored-by: KeXiangWang <[email protected]>

Li0k added 2 commits January 10, 2024 18:55

chore(storage): typo

9671c76

chore(storage): revert num io

58b3bb3

Li0k force-pushed the li0k/storage_task_expire branch from 1faf483 to 58b3bb3 Compare January 10, 2024 11:08

Li0k added 2 commits January 10, 2024 19:25

feat(storage): skip_watermark support task progress

1c4bd73

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

eb7824f

…nto li0k/storage_task_expire

Li0k added 8 commits January 10, 2024 20:03

chore(storage): typo

51f8e6a

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

c43cdd1

…nto li0k/storage_task_expire

fix(storage): fix compile

f8fde47

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

c7ae224

…nto li0k/storage_task_expire

fix(storage): fix compile

99e6e7c

fix(storage): fix compile

a8277a5

fix(storage): move task_progress from skip_iter to merge_iter

b274193

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

35d1bad

…nto li0k/storage_task_expire

hzxa21 reviewed Jan 15, 2024

View reviewed changes

Li0k added 5 commits January 15, 2024 15:18

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

71c12c7

…nto li0k/storage_task_expire

refactor(storage): introduced MonitoredCompactorIterator

ad6f99a

refactor(storage): introduced MonitoredCompactorIterator

074d9b1

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

4819ddd

…nto li0k/storage_task_expire

chore(storage): typo

7ac4049

wenym1 approved these changes Jan 15, 2024

View reviewed changes

fix(storage): address comments

4eb64eb

Li0k enabled auto-merge January 15, 2024 08:31

hzxa21 approved these changes Jan 15, 2024

View reviewed changes

Li0k added this pull request to the merge queue Jan 15, 2024

Merged via the queue into main with commit c9e3e92 Jan 15, 2024
26 of 27 checks passed

Li0k deleted the li0k/storage_task_expire branch January 15, 2024 09:21

fuyufjh added a commit that referenced this pull request Jan 29, 2024

Revert "feat(storage): seperate timeout between process and heartbeat (…

c1f2cc1

…#14366)" This reverts commit c9e3e92.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): seperate timeout between process and heartbeat #14366

feat(storage): seperate timeout between process and heartbeat #14366

Li0k commented Jan 4, 2024 •

edited

Loading

Little-Wallace Jan 5, 2024

Li0k Jan 5, 2024

Little-Wallace Jan 5, 2024

zwang28 left a comment

Little-Wallace left a comment

wenym1 left a comment

wenym1 Jan 5, 2024

Li0k Jan 10, 2024

wenym1 Jan 5, 2024

hzxa21 Jan 9, 2024

Li0k commented Jan 10, 2024

hzxa21 Jan 9, 2024

hzxa21 Jan 15, 2024

hzxa21 Jan 15, 2024

Li0k Jan 15, 2024

wenym1 left a comment

feat(storage): seperate timeout between process and heartbeat #14366

feat(storage): seperate timeout between process and heartbeat #14366

Conversation

Li0k commented Jan 4, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwang28 left a comment

Choose a reason for hiding this comment

Little-Wallace left a comment

Choose a reason for hiding this comment

wenym1 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

Li0k commented Jan 10, 2024

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

wenym1 left a comment

Choose a reason for hiding this comment

Li0k commented Jan 4, 2024 •

edited

Loading