lightning: implement duplicate detector #43460

sleepymole · 2023-04-27T07:32:26Z

What problem does this PR solve?

Issue Number: ref #41629

Problem Summary:

What is changed and how it works?

Implement a duplicate detector to detect key duplicates.

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

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

None

ti-chi-bot · 2023-04-27T07:32:29Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

D3Hunter
lance6716

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2023-04-27T07:32:29Z

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

sleepymole · 2023-05-06T09:49:43Z

/test unit-test

lance6716

will review worker later

lance6716 · 2023-05-06T11:59:33Z

br/pkg/lightning/duplicate/internal_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestInternalKey(t *testing.T) {


Seems it's a good case to try FuzzXxx

But it seems FuzzXxx will not run automatically. If you think we need some fuzz tests, I can add some random tests using rand.

Not really need, I just find a good place to try new technique.

br/pkg/lightning/duplicate/detector_test.go

lance6716

Rest lgtm

br/pkg/lightning/duplicate/detector.go

br/pkg/lightning/duplicate/worker.go

lance6716

rest lgtm

br/pkg/lightning/duplicate/worker.go

br/pkg/lightning/duplicate/detector.go

lance6716 · 2023-05-09T06:22:53Z

br/pkg/lightning/duplicate/worker.go

+		return splitKey
+	}
+
+	c1, c2 := startKey[l], endKey[l]


will endKey equal to empty slice? so we will meet index out of range

It's guaranteed that endKey is greater than startKey here.

br/pkg/lightning/duplicate/worker.go

sleepymole · 2023-05-11T06:02:23Z

@D3Hunter @GMHDBJD PTAL

D3Hunter

rest lgtm

D3Hunter · 2023-05-15T03:33:00Z

br/pkg/lightning/duplicate/detector.go

+// KeyAdder is used to add keys to the Detector.
+type KeyAdder struct {
+	w   extsort.Writer
+	buf []byte


It's used in Add function.

D3Hunter · 2023-05-15T03:38:59Z

br/pkg/lightning/duplicate/detector.go

+
+// Detect detects duplicate keys that have been added to the Detector.
+// It returns the number of duplicate keys detected and any error encountered.
+func (d *Detector) Detect(ctx context.Context, opts *DetectOptions) (numDups int64, _ error) {


can we move Detector code up, close with it's definition

Addressed in c2d6a17.

D3Hunter · 2023-05-15T03:42:45Z

br/pkg/lightning/duplicate/detector.go

+	}
+	opts.ensureDefaults()
+
+	logTask := d.logger.Begin(zap.InfoLevel, "[dup-detector] sort keys")


use a zapper field to indicate [dup-detector] part?

Addressed in c2d6a17.

sleepymole · 2023-05-15T09:35:02Z

/merge

ti-chi-bot · 2023-05-15T09:35:06Z

This pull request has been accepted and is ready to merge.

Commit hash: c2d6a17

ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 27, 2023

sleepymole added the component/lightning This issue is related to Lightning of TiDB. label Apr 27, 2023

sleepymole force-pushed the dup-detector branch from a488e1f to 9d5b799 Compare April 27, 2023 08:30

sleepymole force-pushed the dup-detector branch 9 times, most recently from 4ce7382 to 4a661d2 Compare May 6, 2023 08:29

sleepymole marked this pull request as ready for review May 6, 2023 08:29

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 May 6, 2023

sleepymole force-pushed the dup-detector branch from 4a661d2 to fc27dcc Compare May 6, 2023 08:40

lightning: implement duplciate detector

6e92039

sleepymole force-pushed the dup-detector branch from fc27dcc to 6e92039 Compare May 6, 2023 08:50

sleepymole requested review from lance6716, D3Hunter and GMHDBJD May 6, 2023 09:22

lance6716 reviewed May 6, 2023

View reviewed changes

sleepymole requested a review from lance6716 May 9, 2023 01:59

lance6716 reviewed May 9, 2023

View reviewed changes

br/pkg/lightning/duplicate/detector.go Outdated Show resolved Hide resolved

br/pkg/lightning/duplicate/worker.go Outdated Show resolved Hide resolved

br/pkg/lightning/duplicate/worker.go Show resolved Hide resolved

br/pkg/lightning/duplicate/worker.go Show resolved Hide resolved

lance6716 reviewed May 9, 2023

View reviewed changes

address comments

1dc2ce8

lance6716 approved these changes May 9, 2023

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 9, 2023

lance6716 changed the title ~~lightning: implement duplciate detector~~ lightning: implement duplicate detector May 9, 2023

D3Hunter approved these changes May 15, 2023

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 15, 2023

sleepymole added 2 commits May 15, 2023 17:05

Merge remote-tracking branch 'origin/master' into dup-detector

c663315

address comments

c2d6a17

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label May 15, 2023

ti-chi-bot bot merged commit 433ac5c into pingcap:master May 15, 2023
11 checks passed

sleepymole deleted the dup-detector branch May 15, 2023 09:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lightning: implement duplicate detector #43460

lightning: implement duplicate detector #43460

sleepymole commented Apr 27, 2023 •

edited by D3Hunter

ti-chi-bot bot commented Apr 27, 2023 •

edited

ti-chi-bot bot commented Apr 27, 2023

sleepymole commented May 6, 2023

lance6716 left a comment

lance6716 May 6, 2023

sleepymole May 9, 2023

lance6716 May 9, 2023

lance6716 left a comment

lance6716 left a comment

lance6716 May 9, 2023

sleepymole May 9, 2023

sleepymole commented May 11, 2023

D3Hunter left a comment

D3Hunter May 15, 2023

sleepymole May 15, 2023

D3Hunter May 15, 2023

sleepymole May 15, 2023

D3Hunter May 15, 2023

sleepymole May 15, 2023

sleepymole commented May 15, 2023

ti-chi-bot bot commented May 15, 2023

lightning: implement duplicate detector #43460

lightning: implement duplicate detector #43460

Conversation

sleepymole commented Apr 27, 2023 • edited by D3Hunter

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot bot commented Apr 27, 2023 • edited

ti-chi-bot bot commented Apr 27, 2023

sleepymole commented May 6, 2023

lance6716 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

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleepymole commented May 11, 2023

D3Hunter 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

sleepymole commented May 15, 2023

ti-chi-bot bot commented May 15, 2023

sleepymole commented Apr 27, 2023 •

edited by D3Hunter

ti-chi-bot bot commented Apr 27, 2023 •

edited