Log Backup: decouple log backup resolve locks from GCWorker. by 3pointer · Pull Request #45904 · pingcap/tidb

3pointer · 2023-08-08T07:56:40Z

What problem does this PR solve?

Issue Number: close #40759
Problem Summary:
#35403 introduce 1 minute scan locks of the whole cluster in GCWorker. which will cost CPU when cluster has large number of regions. This PR try to decouple it.

What is changed and how it works?

Record checkpoint and move resolve locks in advancer.
Abstract GCWorkerLockResolver used for LogBackup/GCWorker/GCWorkerTest. move ResolveLocksForRange into gcutil.go to avoid cycle import.

 GCLockResolver is used for GCWorker and log backup advancer to resolve locks.
// #Note: Put it here to avoid cycle import
ype GCLockResolver interface {
 	// ResolveLocks tries to resolve expired locks.
 	// 1. For GCWorker it will scan locks for all regions before *safepoint*,
 	// and force remove locks. rollback the txn, no matter the lock is expired of not.
 	// 2. For log backup advancer, it will scan all locks for a small range.
 	// and it will check status of the txn. resolve the locks if txn is expired, Or do nothing.
 	ResolveLocks(*tikv.Backoffer, []*txnlock.Lock, tikv.RegionVerID) (bool, error)

 	// ScanLocks only used for mock test.
 	ScanLocks([]byte, uint64) []*txnlock.Lock
 	// We need to get tikvStore to build rangerunner.
 	// TODO: the most code is in client.go and the store is only used to locate end keys of a region.
 	// maybe we can move GCLockResolver into client.go.
 	GetStore() tikv.Storage
 }

Remove logbackup logic in GCWorker.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
before this PR, resolve lock every minutes

with this PR. resolve lock every 10 minutes. which will save CPU usage.

and check lock resolved.

Side effects

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

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Decouple log backup resolve locks from GCWorker to save TiKV CPU usage.

ti-chi-bot · 2023-08-08T07:56:44Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tiprow · 2023-08-08T07:57:07Z

Hi @3pointer. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

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 kubernetes/test-infra repository.

br/pkg/streamhelper/advancer.go

3pointer · 2023-08-09T02:01:42Z

/ok-to-test

3pointer · 2023-08-10T02:31:06Z

/ok-to-test

MyonKeminta

The GCWorker part LGTM.

YuJuncen

rest lgtm

YuJuncen · 2023-09-11T06:55:34Z

br/pkg/streamhelper/advancer.go

 	var targets []spans.Valued
-	if c.lastCheckpoint != nil && c.lastCheckpoint.needResolveLocks() {
+	if c.lastCheckpoint != nil && c.lastCheckpoint.needResolveLocks() && !c.inResolvingLock.Load() {
+		c.inResolvingLock.Store(true)


Better to CompareAndSwap here.

tiprow · 2023-09-11T09:19:54Z

@3pointer: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
tiprow_fast_test	`be0c535`	link	true	`/test tiprow_fast_test`

Full PR test history. Your PR dashboard.

Details

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 kubernetes/test-infra repository. I understand the commands that are listed here.

br/pkg/streamhelper/advancer.go

ti-chi-bot · 2023-09-12T06:45:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Leavrth, YuJuncen

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Leavrth,YuJuncen]

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 · 2023-09-12T06:45:14Z

[LGTM Timeline notifier]

Timeline:

2023-09-11 06:55:56.54715264 +0000 UTC m=+252338.471709030: ☑️ agreed by YuJuncen.
2023-09-12 06:45:13.323690821 +0000 UTC m=+338095.248247215: ☑️ agreed by Leavrth.

ti-chi-bot · 2023-09-12T07:26:25Z

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

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

ti-chi-bot · 2023-09-12T07:27:09Z

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

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

…#46893) close #40759

…#46894) close #40759

3pointer added 7 commits August 7, 2023 17:55

update checkpoint

c99737f

refactor GCWorker for lockResolver

6ac2d25

decouple logbackup in GCWorker

418bd47

use GCLockResolver in advancer

1bf06cd

avoid cycle import

a0742dc

expose tikvStore for lockResolver

885425f

update test

e0df995

YuJuncen reviewed Aug 8, 2023

View reviewed changes

br/pkg/streamhelper/advancer.go Outdated Show resolved Hide resolved

YuJuncen reviewed Aug 8, 2023

View reviewed changes

br/pkg/streamhelper/advancer.go Show resolved Hide resolved

address comment

a209893

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Aug 9, 2023

3pointer added 4 commits August 9, 2023 10:32

fix bazel

ae12e5c

remove LocateKey/SendReq in GCLockResolver interface

5e4ba62

fix gc worker unit test

713b16f

add ut for resolve lock

8d821a7

3pointer added 6 commits August 10, 2023 17:39

fix

e76a325

precise resolve lock ranges

2adf397

update log

bf8b1d5

udpate go mod for client-go

1fe03aa

update test

7e02b09

remove useless code

8dc9ce3

3pointer added 4 commits September 7, 2023 15:16

address comment

a545c59

address comment

c41accc

fix ut

ba6c52a

fix unstable test

78ace34

MyonKeminta reviewed Sep 8, 2023

View reviewed changes

YuJuncen approved these changes Sep 11, 2023

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 11, 2023

3pointer added 2 commits September 11, 2023 15:26

address comment

be0c535

Merge branch 'master' into decouple_pitr_for_gc

b55796f

Merge branch 'master' into decouple_pitr_for_gc

2ac41e7

Leavrth approved these changes Sep 12, 2023

View reviewed changes

br/pkg/streamhelper/advancer.go Outdated Show resolved Hide resolved

br/pkg/streamhelper/advancer.go Outdated Show resolved Hide resolved

address comments

5ae1830

ti-chi-bot bot added the lgtm label Sep 12, 2023

ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Sep 12, 2023

ti-chi-bot bot merged commit 8af15b1 into pingcap:master Sep 12, 2023

ti-chi-bot mentioned this pull request Sep 12, 2023

Log Backup: decouple log backup resolve locks from GCWorker. (#45904) #46893

Merged

11 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Sep 12, 2023

This is an automated cherry-pick of pingcap#45904

19feb4d

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

ti-chi-bot mentioned this pull request Sep 12, 2023

Log Backup: decouple log backup resolve locks from GCWorker. (#45904) #46894

Merged

11 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Sep 12, 2023

This is an automated cherry-pick of pingcap#45904

ca5cf77

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

3pointer mentioned this pull request Sep 12, 2023

br: fix the issue that state not set correctly after resolve locks #46903

Merged

12 tasks

This was referenced Sep 12, 2023

br: fix the issue that state not set correctly after resolve locks (#46903) #46915

Closed

br: fix the issue that state not set correctly after resolve locks (#46903) #46916

Closed

ti-chi-bot bot pushed a commit that referenced this pull request Sep 12, 2023

Log Backup: decouple log backup resolve locks from GCWorker. (#45904) (…

ec0ffe8

…#46893) close #40759

ti-chi-bot bot pushed a commit that referenced this pull request Oct 9, 2023

Log Backup: decouple log backup resolve locks from GCWorker. (#45904) (…

c82570c

…#46894) close #40759

Conversation

3pointer commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot bot commented Aug 8, 2023

Uh oh!

tiprow bot commented Aug 8, 2023

Uh oh!

Uh oh!

Uh oh!

3pointer commented Aug 9, 2023

Uh oh!

3pointer commented Aug 10, 2023

Uh oh!

MyonKeminta left a comment

Choose a reason for hiding this comment

Uh oh!

YuJuncen left a comment

Choose a reason for hiding this comment

Uh oh!

YuJuncen Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

tiprow bot commented Sep 11, 2023

Uh oh!

Uh oh!

Uh oh!

ti-chi-bot bot commented Sep 12, 2023

Uh oh!

ti-chi-bot bot commented Sep 12, 2023

[LGTM Timeline notifier]

Uh oh!

ti-chi-bot commented Sep 12, 2023

Uh oh!

ti-chi-bot commented Sep 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

3pointer commented Aug 8, 2023 •

edited

Loading