KVStore: Fix spurious region overlap when two region are both applying snapshots #9330

CalvinNeo · 2024-08-16T07:16:45Z

What problem does this PR solve?

Issue Number: close #9329

Problem Summary:

See the issue.

What is changed and how it works?

Ask for proxy's region local state to determine whether regions are actually overlapping, if the overlapped regions are in Applying state.
Clean only the range of new_region rather than old_region.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

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

Documentation

Release note

Fix spurious region overlap problem
修复虚假的 Region overlap 引起的 panic 问题

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

dbms/src/Storages/KVStore/MultiRaft/ApplySnapshot.cpp

dbms/src/Storages/KVStore/MultiRaft/RegionsRangeIndex.cpp

JaySon-Huang · 2024-08-16T07:52:54Z

dbms/src/Storages/KVStore/MultiRaft/ApplySnapshot.cpp

+                        dm_storage->deleteRange(new_key_range, context.getSettingsRef());
                        // We must flush the deletion to the disk here, because we only flush new range when persisting this region later.
-                        dm_storage->flushCache(context, old_key_range, /*try_until_succeed*/ true);
+                        dm_storage->flushCache(context, new_key_range, /*try_until_succeed*/ true);


Why here is changed to delete the new_key_range?

Consider:

apply snapshot a of range [0..100)

apply snapshot b of range [50..100)

apply snapshot a' of range [0..50)

In 3, we could clean the old range [0..100), which covers the written data of snapshot b in stage 2

If we only need to delete the existing data on the new_range, then the following code of ingestFiles(..., /*clear_data_in_range=*/true, ...) can do it. We don't need this branch.

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

dbms/src/Storages/KVStore/MultiRaft/ApplySnapshot.cpp

dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp

dbms/src/Storages/KVStore/MultiRaft/ApplySnapshot.cpp

dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp

JaySon-Huang · 2024-08-19T02:47:17Z

Are these failed tests related to your changes?
https://do.pingcap.net/jenkins/blue/organizations/jenkins/pingcap%2Ftiflash%2Fpull_unit_test/detail/pull_unit_test/1258/pipeline

FAILED TESTS (3/2852):
    7123 ms: /tiflash/gtests_dbms RegionKVStoreTestFAP.FAPWorkerException
    2614 ms: /tiflash/gtests_dbms ByIngestUsingSplit/ApplySnapshotTest.WithNewRegionRange/0
    2657 ms: /tiflash/gtests_dbms ByIngestUsingSplit/ApplySnapshotTest.WithNewRegionRange/1

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

Co-authored-by: JaySon <tshent@qq.com>

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

… fix-simu-snapshot

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

dbms/src/Storages/KVStore/MultiRaft/RegionsRangeIndex.h

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

Co-authored-by: Lloyd-Pottiger <60744015+Lloyd-Pottiger@users.noreply.github.com>

dbms/src/Storages/KVStore/MultiRaft/ApplySnapshot.cpp

dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp

Co-authored-by: JaySon <tshent@qq.com>

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

CalvinNeo · 2024-08-20T04:02:33Z

/hold

… fix-simu-snapshot

CalvinNeo · 2024-08-20T05:16:42Z

/unhold

JaySon-Huang

Rest LGTM

dbms/src/Storages/KVStore/KVStore.h

dbms/src/Storages/KVStore/MultiRaft/ApplySnapshot.cpp

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

JaySon-Huang

LGTM

ti-chi-bot · 2024-08-20T06:57:11Z

[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-20T06:57:13Z

[LGTM Timeline notifier]

Timeline:

2024-08-19 09:19:58.258556397 +0000 UTC m=+170793.393006516: ☑️ agreed by Lloyd-Pottiger.
2024-08-20 06:57:12.751179727 +0000 UTC m=+248627.885629847: ☑️ agreed by JaySon-Huang.

…g snapshots (pingcap#9330) close pingcap#9329 Signed-off-by: Calvin Neo <calvinneo1995@gmail.com> Co-authored-by: JaySon <tshent@qq.com> Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>

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

ti-chi-bot · 2024-11-08T05:48:23Z

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

main

7eb2fc2

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/needs-triage-completed labels Aug 16, 2024

JaySon-Huang reviewed Aug 16, 2024

View reviewed changes

dbms/src/Storages/KVStore/MultiRaft/ApplySnapshot.cpp Outdated Show resolved Hide resolved

JaySon-Huang reviewed Aug 16, 2024

View reviewed changes

dbms/src/Storages/KVStore/MultiRaft/RegionsRangeIndex.cpp Outdated Show resolved Hide resolved

JaySon-Huang reviewed Aug 16, 2024

View reviewed changes

CalvinNeo changed the title ~~KVStore: Fix spurious region overlap when two region are both applying snapshots~~ WIP KVStore: Fix spurious region overlap when two region are both applying snapshots Aug 16, 2024

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

CalvinNeo added 2 commits August 16, 2024 16:57

some tests

cfe7355

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

fix tests

a47e825

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

CalvinNeo changed the title ~~WIP KVStore: Fix spurious region overlap when two region are both applying snapshots~~ KVStore: Fix spurious region overlap when two region are both applying snapshots Aug 16, 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 16, 2024

fix

b522f3b

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

JaySon-Huang reviewed Aug 19, 2024

View reviewed changes

CalvinNeo and others added 6 commits August 19, 2024 11:43

fix test

b131c1b

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

Update dbms/src/Storages/KVStore/MultiRaft/ApplySnapshot.cpp

3f0fb36

Co-authored-by: JaySon <tshent@qq.com>

add logs

ebf7417

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

more logs

4dea331

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

Merge branch 'fix-simu-snapshot' of github.com:CalvinNeo/tiflash into…

5a59b29

… fix-simu-snapshot

more logs

f311227

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

CalvinNeo requested a review from JaySon-Huang August 19, 2024 04:48

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 19, 2024

CalvinNeo requested review from JinheLin and Lloyd-Pottiger August 19, 2024 08:29

Lloyd-Pottiger reviewed Aug 19, 2024

View reviewed changes

dbms/src/Storages/KVStore/MultiRaft/RegionsRangeIndex.h Outdated Show resolved Hide resolved

a

2f4ce7d

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

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

Update dbms/src/Storages/KVStore/MultiRaft/RegionRangeKeys.h

14007a1

Co-authored-by: Lloyd-Pottiger <60744015+Lloyd-Pottiger@users.noreply.github.com>

JaySon-Huang reviewed Aug 19, 2024

View reviewed changes

dbms/src/Storages/KVStore/MultiRaft/ApplySnapshot.cpp Show resolved Hide resolved

JaySon-Huang reviewed Aug 19, 2024

View reviewed changes

dbms/src/Storages/KVStore/MultiRaft/ApplySnapshot.cpp Outdated Show resolved Hide resolved

JaySon-Huang reviewed Aug 19, 2024

View reviewed changes

dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp Outdated Show resolved Hide resolved

CalvinNeo and others added 2 commits August 19, 2024 23:32

Update dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp

ad817bf

Co-authored-by: JaySon <tshent@qq.com>

fix

e6961eb

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2024

Merge branch 'fix-simu-snapshot' of github.com:CalvinNeo/tiflash into…

35f4893

… fix-simu-snapshot

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2024

JaySon-Huang reviewed Aug 20, 2024

View reviewed changes

dbms/src/Storages/KVStore/KVStore.h Outdated Show resolved Hide resolved

dbms/src/Storages/KVStore/MultiRaft/ApplySnapshot.cpp Outdated Show resolved Hide resolved

JaySon-Huang and others added 4 commits August 20, 2024 13:59

Apply suggestions from code review

5709ff4

Merge branch 'master' into fix-simu-snapshot

293c691

a

8e2b10d

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

a

1e6118e

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>

JaySon-Huang approved these changes Aug 20, 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 20, 2024

ti-chi-bot bot merged commit 5500b35 into pingcap:master Aug 20, 2024
5 checks passed

CalvinNeo mentioned this pull request Nov 7, 2024

KVStore: Fix spurious region overlap when two region are both applying snapshots #9592

Merged

12 tasks

ti-chi-bot bot added the needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. label Nov 8, 2024

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

This is an automated cherry-pick of pingcap#9330

76f1d14

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

ti-chi-bot mentioned this pull request Nov 8, 2024

KVStore: Fix spurious region overlap when two region are both applying snapshots (#9330) #9596

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KVStore: Fix spurious region overlap when two region are both applying snapshots #9330

KVStore: Fix spurious region overlap when two region are both applying snapshots #9330

CalvinNeo commented Aug 16, 2024 •

edited

Loading

JaySon-Huang Aug 16, 2024

CalvinNeo Aug 16, 2024

JaySon-Huang Aug 19, 2024

CalvinNeo Aug 19, 2024

JaySon-Huang commented Aug 19, 2024 •

edited

Loading

CalvinNeo commented Aug 20, 2024

CalvinNeo commented Aug 20, 2024

JaySon-Huang left a comment

JaySon-Huang left a comment

ti-chi-bot bot commented Aug 20, 2024

ti-chi-bot bot commented Aug 20, 2024

ti-chi-bot commented Nov 8, 2024

KVStore: Fix spurious region overlap when two region are both applying snapshots #9330

KVStore: Fix spurious region overlap when two region are both applying snapshots #9330

Conversation

CalvinNeo commented Aug 16, 2024 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

JaySon-Huang Aug 16, 2024

Choose a reason for hiding this comment

CalvinNeo Aug 16, 2024

Choose a reason for hiding this comment

JaySon-Huang Aug 19, 2024

Choose a reason for hiding this comment

CalvinNeo Aug 19, 2024

Choose a reason for hiding this comment

JaySon-Huang commented Aug 19, 2024 • edited Loading

CalvinNeo commented Aug 20, 2024

CalvinNeo commented Aug 20, 2024

JaySon-Huang left a comment

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 20, 2024

ti-chi-bot bot commented Aug 20, 2024

[LGTM Timeline notifier]

ti-chi-bot commented Nov 8, 2024

CalvinNeo commented Aug 16, 2024 •

edited

Loading

JaySon-Huang commented Aug 19, 2024 •

edited

Loading