load data: physical mode part1 #42817

D3Hunter · 2023-04-04T10:25:55Z

What problem does this PR solve?

Issue Number: ref #40499

Problem Summary:

What is changed and how it works?

ingegrate lightning physical mode into load data

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-04T10:25:57Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

GMHDBJD
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.

GMHDBJD · 2023-04-06T02:17:13Z

executor/importer/chunk_process.go

+
+	checksum verify.KVChecksum
+	encoder  kvEncoder
+	kvStore  tidbkv.Storage


can we use codec instead of kvstore？

lightning uses it to alloc auto-id in common.AllocGlobalAutoID too, but this place can be removed

hawkingrei · 2023-04-06T08:05:00Z

/test all

hawkingrei · 2023-04-06T08:12:19Z

/retest

GMHDBJD · 2023-04-06T09:08:20Z

executor/importer/table_import.go

+	tableMeta := &mydump.MDTableMeta{
+		DB:        e.DBName,
+		Name:      e.Table.Meta().Name.O,
+		DataFiles: e.toMyDumpFiles(),


Add TODO for IsRowOrdered?

D3Hunter · 2023-04-06T10:27:48Z

/retest

GMHDBJD

LGTM

lance6716

5 / 11 files viewed

lance6716 · 2023-04-06T11:36:40Z

executor/importer/chunk_process.go

+
+func (b *deliverKVBatch) add(kvs *kv.Pairs) {
+	for _, pair := range kvs.Pairs {
+		if pair.Key[tablecodec.TableSplitKeyLen+1] == 'r' {


Maybe we can use tablecodec.IsRecordKey

fix comments

lance6716 · 2023-04-06T11:40:58Z

executor/importer/chunk_process.go

+	deliverCompleteCh := make(chan deliverResult)
+	go func() {
+		defer close(deliverCompleteCh)
+		err := p.deliverLoop(ctx)


Not important, but I prefer the error group pattern. When the job is finished, close the channel to notify the consumer. When any loop meets error, error group will automatically cancel the derived context and the error can be found by eg.Wait()

simplified from lightning code, will add a comment and do it later

D3Hunter · 2023-04-06T12:02:44Z

/retest

D3Hunter · 2023-04-07T02:28:27Z

/retest

D3Hunter · 2023-04-07T02:48:41Z

multiple-valued index related test will add later

lance6716

rest lgtm

lance6716 · 2023-04-07T03:27:58Z

tests/realtikvtest/loaddatatest/load_data_test.go

+	s.tk.MustExec("use " + db)
+}
+
+func (s *mockGCSSuite) TestPhysicalMode() {


In future we can move some tests to a common package, and let different backend share the tests

physical related can only run in realtikvtest(check_dev_2), we can move logical test here.

lance6716 · 2023-04-07T03:53:41Z

executor/importer/table_import.go

+	tidbCfg := tidb.GetGlobalConfig()
+	// todo: add job id too
+	sortPathSuffix := "import-" + strconv.Itoa(int(tidbCfg.Port))
+	sortPath := filepath.Join(tidbCfg.TempDir, sortPathSuffix)


I found both TempDir and TempStoragePath and they don't have comment, not sure how to use them. @tangenta can you help us?

And will lightning's builtin disk quota conflicts with TiDB's?

copied from here

tidb/ddl/ingest/env.go

Line 88 in 7c2c992

sortPath := filepath.Join(tidbCfg.TempDir, sortPathSuffix)

will lightning's builtin disk quota conflicts with TiDB

yes, we should precheck whether's ongoing add-index.

No a big problem, we can check it later @tangenta PTAL at your convenience

lance6716 · 2023-04-07T03:56:26Z

executor/importer/table_import.go

+		ConnCompressType:        config.CompressionNone,
+		WorkerConcurrency:       config.DefaultRangeConcurrency * 2,
+		KVWriteBatchSize:        config.KVWriteBatchSize,
+		CheckpointEnabled:       true,


We don't need to turn on checkpoint now?

local backend report error when the sort-dir already exists & checkpoint disabled

will add a todo and fix it later

lance6716 · 2023-04-07T03:58:14Z

executor/importer/table_import.go

+		DupeDetectEnabled:       false,
+		DuplicateDetectOpt:      local.DupDetectOpt{ReportErrOnDup: false},
+		StoreWriteBWLimit:       0,
+		ShouldCheckWriteStall:   false,


This should be true because we don't periodically switch import mode so the cluster may be slow to ingest. Today I meet a ONCALL 😭

D3Hunter · 2023-04-07T04:52:58Z

/merge

ti-chi-bot · 2023-04-07T04:53:01Z

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

Commit hash: 66d0c78

D3Hunter · 2023-04-07T06:56:59Z

/retest

D3Hunter added 11 commits April 4, 2023 18:21

physical mode

61fdcc0

physical

c65610e

physical

aa86eb6

physical

9322892

physical import

c9bc53a

physical

cd9560c

physical i-t

689d7db

physical i-t

4b6e90e

physical

f53949b

physical

91abd22

physical

d866467

ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 4, 2023

D3Hunter requested a review from lance6716 April 4, 2023 10:26

ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 4, 2023

D3Hunter requested a review from GMHDBJD April 4, 2023 10:26

D3Hunter changed the title ~~load data: physical mode~~ load data: physical mode part1 Apr 4, 2023

D3Hunter added 3 commits April 4, 2023 21:36

fix lint

6111744

fix bazel

3fd38bb

fix lint

38b435b

GMHDBJD reviewed Apr 6, 2023

View reviewed changes

fix comments

d902ea6

GMHDBJD reviewed Apr 6, 2023

View reviewed changes

D3Hunter added 3 commits April 6, 2023 17:43

fix comments

6ff4527

Merge remote-tracking branch 'origin/master' into physical-mode

e1b3679

bazel

85801b9

GMHDBJD approved these changes Apr 6, 2023

View reviewed changes

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

lance6716 reviewed Apr 6, 2023

View reviewed changes

fix comments

c0dbaee

lance6716 reviewed Apr 7, 2023

View reviewed changes

lance6716 approved these changes Apr 7, 2023

View reviewed changes

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

fix comments

66d0c78

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

ti-chi-bot merged commit 2af85d1 into master Apr 7, 2023

ti-chi-bot deleted the physical-mode branch April 7, 2023 07:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

load data: physical mode part1 #42817

load data: physical mode part1 #42817

D3Hunter commented Apr 4, 2023

ti-chi-bot commented Apr 4, 2023 •

edited

Loading

GMHDBJD Apr 6, 2023

D3Hunter Apr 6, 2023 •

edited

Loading

hawkingrei commented Apr 6, 2023

hawkingrei commented Apr 6, 2023

GMHDBJD Apr 6, 2023

D3Hunter Apr 6, 2023

D3Hunter commented Apr 6, 2023

GMHDBJD left a comment

lance6716 left a comment

lance6716 Apr 6, 2023

D3Hunter Apr 6, 2023

lance6716 Apr 6, 2023 •

edited

Loading

D3Hunter Apr 6, 2023

D3Hunter commented Apr 6, 2023

D3Hunter commented Apr 7, 2023

D3Hunter commented Apr 7, 2023

lance6716 left a comment

lance6716 Apr 7, 2023

D3Hunter Apr 7, 2023

lance6716 Apr 7, 2023

D3Hunter Apr 7, 2023

lance6716 Apr 7, 2023

lance6716 Apr 7, 2023

D3Hunter Apr 7, 2023

lance6716 Apr 7, 2023

D3Hunter commented Apr 7, 2023

ti-chi-bot commented Apr 7, 2023

D3Hunter commented Apr 7, 2023

load data: physical mode part1 #42817

load data: physical mode part1 #42817

Conversation

D3Hunter commented Apr 4, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

D3Hunter Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

hawkingrei commented Apr 6, 2023

hawkingrei commented Apr 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D3Hunter commented Apr 6, 2023

GMHDBJD 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

lance6716 Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D3Hunter commented Apr 6, 2023

D3Hunter commented Apr 7, 2023

D3Hunter commented Apr 7, 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

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

D3Hunter commented Apr 7, 2023

ti-chi-bot commented Apr 7, 2023

D3Hunter commented Apr 7, 2023

ti-chi-bot commented Apr 4, 2023 •

edited

Loading

D3Hunter Apr 6, 2023 •

edited

Loading

lance6716 Apr 6, 2023 •

edited

Loading