Disagg: Fix the issue that lifetime of read-snapshots in WN may be longer than expected #9297

JinheLin · 2024-08-07T09:25:43Z

What problem does this PR solve?

Issue Number: close #9298

Problem summary:
Snapshot of each SegmentReadTask will be released after FetchDisaggPages is called. But for some segments with no ColumnFileTiny or ColumnFileInMemory to fetch, FetchDisaggPages will never be called.

What is changed and how it works?

1. In WN, release snapshots of SegmentReadTasks that do not have ColumnFileTiny and ColumnFileInMemory.
2. In CN, no matter all ColumnFileTinys and ColumnFileInMemorys are cached or not, call FetchDisaggPages to release snapshot in WN.

Notes:

From v8.1.0, CN need to fetch ColumnFileTiny and ColumnFileInMemory.
In v7.5.0, CN only need to fetch ColumnFileTiny.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
- Run CH-Benchmark workload and check the snapshot metrics.
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Fix the issue that lifetime of read-snapshots in write-nodes may be longer than expected in disaggregated-mode.

JinheLin · 2024-08-08T02:18:25Z

/retest

…lizing the snapshot data.

breezewish · 2024-08-12T13:36:58Z

dbms/src/Storages/DeltaMerge/SegmentReadTask.cpp

-    if (request.page_ids_size() == 0 && !needFetchMemTableSet())
-        return;
-
+    // No matter all delta data is cached or not, call FetchDisaggPages to release snapshot in WN.


Will this cause optimizations like in https://github.com/tidbcloud/tiflash-cse/pull/236/files get reverted? (The PR skips occupySpace to reduce lock contention)

@JinheLin @breezewish
What about adding another unary RPC method "CancelDisaggTask". When the MPP request on CN is finished or canceled, CN calls "CancelDisaggTask" to let the WN release the whole snapshot. Instead of sending "fetchPages" segment by segment?

This can

decouple the optimization of 'fetchPages' and release WN snapshot

reduce the total RPC calls between CN and RN

handle the situation when the request is canceled in the CN

Are you referring to this code: https://github.com/tidbcloud/tiflash-cse/pull/236/files#diff-415598e006988b3cadcac5e3a4eda7fdbed5ac29cc1ce8d11211a5d26a4d8449 ?

If there are no ColumnFileTiny to fetch, skip here: https://github.com/pingcap/tiflash/blob/master/dbms/src/Storages/DeltaMerge/SegmentReadTask.cpp#L332.

Code in https://github.com/pingcap/tiflash/pull/9297/files/c1e5333660e180072ff658e2cd45349b6d2db28b#diff-9fc375427641c2044d5949241ef2f173ee8b4d24e252216a6cba0018ad5c0413L545-L547 means all pages are cached.

@JinheLin Thanks, it looks good!

@JaySon-Huang

RPCs that can be reduced is when all pages are cached and there is no ColumnFileInMemory since v8.1, because ColumnFileInMemory is not cached.

If there are no write, segments are stable-only, snapshots are released quickly in WN.

If there are some writes, the probability that there are no ColumnFileInMemory is low, so these RPCs likely cannot be reduced.

The coupling problem also looks fine because the logic is simple, and this code does not spread outward.

The situation that CancelDisaggTask to handle is abnormal requests. CancelDisaggTask can only serve as a fallback. If a request takes a long time to execute, the lifecycle of the snapshot will still be long than expected.

Another optimization is use just one stream RPC to fetch pages of all segments. But this should be quite troublesome.

JaySon-Huang

LGTM

ti-chi-bot · 2024-08-13T03:58:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JaySon-Huang, Lloyd-Pottiger

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JaySon-Huang,Lloyd-Pottiger]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-08-13T03:58:32Z

[LGTM Timeline notifier]

Timeline:

2024-08-12 09:44:45.602379813 +0000 UTC m=+174770.305849456: ☑️ agreed by Lloyd-Pottiger.
2024-08-13 03:58:31.709183625 +0000 UTC m=+240396.412653291: ☑️ agreed by JaySon-Huang.

ti-chi-bot · 2024-08-13T04:12:34Z

@JinheLin: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

trigger some heavy tests which will not run always when PR updated.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2024-08-13T05:52:19Z

In response to a cherrypick label: new pull request created to branch release-8.1: #9316.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2024-08-13T05:52:21Z

In response to a cherrypick label: new pull request created to branch release-7.5: #9317.

…nger than expected (#9297) (#9317) close #9298 1. In WN, release snapshots of SegmentReadTasks that do not have ColumnFileTiny and ColumnFileInMemory. 2. In CN, no matter all ColumnFileTinys and ColumnFileInMemorys are cached or not, call FetchDisaggPages to release snapshot in WN. Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io> Co-authored-by: jinhelin <linjinhe33@gmail.com>

…nger than expected (#9297) (#9316) close #9298 1. In WN, release snapshots of SegmentReadTasks that do not have ColumnFileTiny and ColumnFileInMemory. 2. In CN, no matter all ColumnFileTinys and ColumnFileInMemorys are cached or not, call FetchDisaggPages to release snapshot in WN. Co-authored-by: jinhelin <linjinhe33@gmail.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

JinheLin changed the title ~~WIP: Disagg: Release snapshots of stable-only SegmentReadTasks after serializing the snapshot data~~ WIP: Disagg: Release snapshots of stable-only SegmentReadTasks after serializing snapshot data Aug 8, 2024

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 8, 2024

JinheLin changed the title ~~WIP: Disagg: Release snapshots of stable-only SegmentReadTasks after serializing snapshot data~~ WIP: Disagg: Fix the issue that lifetime of read-snapshots in WN may be longer than expected. Aug 8, 2024

JinheLin changed the title ~~WIP: Disagg: Fix the issue that lifetime of read-snapshots in WN may be longer than expected.~~ WIP: Disagg: Fix the issue that lifetime of read-snapshots in WN may be longer than expected Aug 8, 2024

JinheLin force-pushed the fix_disagg_snap_with_no_delta branch 2 times, most recently from 3cbfaa7 to 09a2ccd Compare August 9, 2024 10:02

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 9, 2024

JinheLin force-pushed the fix_disagg_snap_with_no_delta branch from 36d5d78 to 9dc0288 Compare August 12, 2024 03:56

JinheLin added 5 commits August 12, 2024 12:50

Disagg: Release snapshots of stable-only SegmentReadTasks after seria…

960859a

…lizing the snapshot data.

ci

ef5b659

ci

f5290b1

add comment

eb22ac1

add comments

c1e5333

JinheLin force-pushed the fix_disagg_snap_with_no_delta branch from 7769eb6 to c1e5333 Compare August 12, 2024 04:50

JinheLin changed the title ~~WIP: Disagg: Fix the issue that lifetime of read-snapshots in WN may be longer than expected~~ Disagg: Fix the issue that lifetime of read-snapshots in WN may be longer than expected Aug 12, 2024

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 12, 2024

JinheLin requested review from breezewish, JaySon-Huang and Lloyd-Pottiger and removed request for breezewish August 12, 2024 06:08

Lloyd-Pottiger approved these changes Aug 12, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Aug 12, 2024

breezewish reviewed Aug 12, 2024

View reviewed changes

JaySon-Huang approved these changes Aug 13, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 13, 2024

Merge branch 'master' into fix_disagg_snap_with_no_delta

358f985

ti-chi-bot bot merged commit c6fcddf into pingcap:master Aug 13, 2024
5 checks passed

ti-chi-bot added needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. labels Aug 13, 2024

ti-chi-bot mentioned this pull request Aug 13, 2024

Disagg: Fix the issue that lifetime of read-snapshots in WN may be longer than expected (#9297) #9316

Merged

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Aug 13, 2024

This is an automated cherry-pick of pingcap#9297

9c74918

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Aug 13, 2024

Disagg: Fix the issue that lifetime of read-snapshots in WN may be longer than expected (#9297) #9317

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disagg: Fix the issue that lifetime of read-snapshots in WN may be longer than expected #9297

Disagg: Fix the issue that lifetime of read-snapshots in WN may be longer than expected #9297

JinheLin commented Aug 7, 2024 •

edited

Loading

JinheLin commented Aug 8, 2024

breezewish Aug 12, 2024 •

edited

Loading

JaySon-Huang Aug 13, 2024

JinheLin Aug 13, 2024

breezewish Aug 13, 2024

JinheLin Aug 13, 2024 •

edited

Loading

JinheLin Aug 13, 2024 •

edited

Loading

JaySon-Huang Aug 13, 2024

JaySon-Huang left a comment

ti-chi-bot bot commented Aug 13, 2024

ti-chi-bot bot commented Aug 13, 2024

ti-chi-bot bot commented Aug 13, 2024

ti-chi-bot commented Aug 13, 2024

ti-chi-bot commented Aug 13, 2024

Disagg: Fix the issue that lifetime of read-snapshots in WN may be longer than expected #9297

Disagg: Fix the issue that lifetime of read-snapshots in WN may be longer than expected #9297

Conversation

JinheLin commented Aug 7, 2024 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

JinheLin commented Aug 8, 2024

breezewish Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

JaySon-Huang Aug 13, 2024

Choose a reason for hiding this comment

JinheLin Aug 13, 2024

Choose a reason for hiding this comment

breezewish Aug 13, 2024

Choose a reason for hiding this comment

JinheLin Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

JinheLin Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

JaySon-Huang Aug 13, 2024

Choose a reason for hiding this comment

JaySon-Huang left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Aug 13, 2024

ti-chi-bot bot commented Aug 13, 2024

[LGTM Timeline notifier]

ti-chi-bot bot commented Aug 13, 2024

ti-chi-bot commented Aug 13, 2024

ti-chi-bot commented Aug 13, 2024

JinheLin commented Aug 7, 2024 •

edited

Loading

breezewish Aug 12, 2024 •

edited

Loading

JinheLin Aug 13, 2024 •

edited

Loading

JinheLin Aug 13, 2024 •

edited

Loading