ddl/ingest: add the management infra for lightning struct #37521

tangenta · 2022-08-31T13:01:59Z

What problem does this PR solve?

Issue Number: ref #35983

What is changed and how it works?

This PR encapsulate the lightning structs such as Backend, Engine and Writer, so that it could be easily used by onCreateIndex().

Check List

Tests

It is not easy to write unit tests because it needs the cluster environment.

The integration test has been added in #37320. We will switch the add index process after the whole development is finished & merged.

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 · 2022-08-31T13:02:01Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

xiongjiwei
zimulala

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 · 2022-08-31T13:17:03Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/693c43347da1b1e78789d42cacd309cd39858684

zimulala · 2022-09-02T07:40:29Z

sessionctx/variable/varsutil.go

@@ -318,6 +318,15 @@ func TidbOptInt64(opt string, defaultVal int64) int64 {
 	return val
 }

+// TidbOptUint64 converts a string to an uint64.
+func TidbOptUint64(opt string, defaultVal uint64) uint64 {


s/TidbOptUint64/TiDBOptUint64

This is consistent with TidbOptInt64, TidbOptInt and tidbOptFloat64, should I rename the others?

ddl/lightning/backend.go

ddl/lightning/message.go

zimulala · 2022-09-02T09:02:00Z

ddl/lightning/engine_mgr.go

+	// Calculate lightning concurrency degree and set memory usage
+	// and pre-allocate memory usage for worker.
+	m.MemRoot.RefreshConsumption()
+	ok := m.MemRoot.TestConsume(int64(bc.cfg.TikvImporter.LocalWriterMemCacheSize))


Maybe we can rename this function TestConsume, like checkConsume or other names. The name confused me a little bit, thinking it was code for testing.

It is suggested by @xiongjiwei ... let me change it to CheckConsume.

ddl/lightning/engine_mgr.go

zimulala · 2022-09-02T09:12:54Z

ddl/lightning/engine_mgr.go

+	ei.Clean()
+	m.Delete(indexID)
+	m.MemRoot.ReleaseWithTag(encodeEngineTag(jobID, indexID))
+	m.MemRoot.Release(StructSizeWriterCtx * int64(ei.writerCount))


I don't see Consume StructSizeWriterCtx in Register, so why do we need to release it here?

Done. b1bfdba

ddl/lightning/mem_root.go

ddl/lightning/env.go

ddl/lightning/message.go

ddl/lightning/config.go

Co-authored-by: Lynn <zimu_xia@126.com>

zimulala · 2022-09-05T03:04:44Z

ddl/lightning/backend_mgr.go

+	}
+}
+
+type glueLit struct{}


Please add comments for glueLit.
Besides, do we need to implement this struct's functions?

do we need to implement this struct's functions?

No, it is a noop struct.

ddl/lightning/config.go

zimulala · 2022-09-05T03:48:19Z

ddl/lightning/backend_mgr.go

+	if !exist {
+		return
+	}
+	bc.EngMgr.UnregisterAll(jobID)


I don't see where EngMgr.Register is called. Has it not been called yet or am I reading it wrong?

The engineInfo is initialized in writePhysicalTableRecord. It will be used in the next PR:

https://github.com/pingcap/tidb/pull/36767/files#diff-1bbc7833321782b3a22a842b6237974b6a60cb07b49b246b85552ab5e915ed6eR757

ddl/lightning/engine.go

zimulala · 2022-09-05T06:56:03Z

ddl/lightning/env.go

@@ -0,0 +1,115 @@
+// Copyright 2022 PingCAP, Inc.


Could we add some UT for this file?

Updated. BTW genRLimit is a wrapper for system call, it is meaningless to test it..

tangenta · 2022-09-05T10:21:09Z

/merge

ti-chi-bot · 2022-09-05T10:21:13Z

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

Commit hash: 9d02ffb

tangenta · 2022-09-05T11:26:51Z

/hold cc @pingcap/tidb-configuration-reviewer

Benjamin2037

LGTM

ti-chi-bot · 2022-09-05T14:03:52Z

@Benjamin2037: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM

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.

ddl/ingest/BUILD.bazel

bb7133

LGTM

bb7133 · 2022-09-05T15:35:59Z

/merge

ti-chi-bot · 2022-09-05T15:36:03Z

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

Commit hash: 51f1095

bb7133 · 2022-09-05T15:36:13Z

/hold

tangenta · 2022-09-06T03:57:59Z

/unhold

sre-bot · 2022-09-06T04:52:29Z

TiDB MergeCI notify

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci/integration-cdc-test	🟢 all 37 tests passed	26 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	12 min	Existing passed
idc-jenkins-ci-tidb/integration-common-test	🟢 all 17 tests passed	11 min	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	6 min 59 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	4 min 59 sec	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	4 min 45 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	4 min 22 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 11 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	3 min 0 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

ddl/lightning: add the management infra for lighting object

3f8886c

tangenta requested a review from a team as a code owner August 31, 2022 13:02

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 31, 2022

tangenta mentioned this pull request Aug 31, 2022

Improve the performance of adding index #35983

Closed

20 tasks

tangenta added 3 commits August 31, 2022 21:26

refine the structure

1402e50

rename workerCtx to writerCtx

2b237aa

remove unnecessary code

693c433

tangenta requested review from zimulala, xiongjiwei and Benjamin2037 September 1, 2022 09:11

zimulala reviewed Sep 2, 2022

View reviewed changes

xiongjiwei reviewed Sep 2, 2022

View reviewed changes

ddl/lightning/mem_root.go Show resolved Hide resolved

ddl/lightning/env.go Outdated Show resolved Hide resolved

ddl/lightning/env.go Outdated Show resolved Hide resolved

ddl/lightning/message.go Outdated Show resolved Hide resolved

ddl/lightning/config.go Outdated Show resolved Hide resolved

tangenta and others added 9 commits September 2, 2022 18:28

Update ddl/lightning/backend.go

64bf089

Co-authored-by: Lynn <zimu_xia@126.com>

ddl/lightning: rename TestConsume to CheckConsume

d5444f0

add more info to the log

b7b7103

ddl/lightning: remove unused messages

5aaa6fb

remove unnecessary variable

b1bfdba

ddl/lightning: remove unnecessary if

5b7817c

ddl/lightning: rename logAllocMemFailed to genAllocMemFailedErr

c58d2ad

ddl/lightning: don't use sort_buffer_size and hardcode to 2GB

e1a67a9

ddl/lightning: remove unused function genMaxMemoryLimit

d65c0b5

zimulala reviewed Sep 5, 2022

View reviewed changes

address comment

37eef0c

zimulala reviewed Sep 5, 2022

View reviewed changes

tangenta added 3 commits September 5, 2022 17:30

ddl/lightning: log job ID when an error occurred

3a5cd49

add unit test for genLightningDataDir

393a165

Merge branch 'master' into add-index-lit

aca6609

ti-chi-bot added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 5, 2022

fix linter

9d02ffb

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 5, 2022

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

rename ddl/lightning to ddl/ingest

c6c195e

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Sep 5, 2022

fix build

b6f810e

tangenta changed the title ~~ddl/lightning: add the management infra for lightning struct~~ ddl/ingest: add the management infra for lightning struct Sep 5, 2022

Benjamin2037 approved these changes Sep 5, 2022

View reviewed changes

Benjamin2037 reviewed Sep 5, 2022

View reviewed changes

ddl/ingest/BUILD.bazel Outdated Show resolved Hide resolved

Benjamin2037 reviewed Sep 5, 2022

View reviewed changes

ddl/ingest/BUILD.bazel Outdated Show resolved Hide resolved

Benjamin2037 reviewed Sep 5, 2022

View reviewed changes

ddl/ingest/BUILD.bazel Outdated Show resolved Hide resolved

Benjamin2037 reviewed Sep 5, 2022

View reviewed changes

ddl/ingest/BUILD.bazel Outdated Show resolved Hide resolved

update bazel

51f1095

bb7133 approved these changes Sep 5, 2022

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 5, 2022

Merge branch 'master' into add-index-lit

ee5b7f9

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

ti-chi-bot merged commit 878ac8e into pingcap:master Sep 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl/ingest: add the management infra for lightning struct #37521

ddl/ingest: add the management infra for lightning struct #37521

tangenta commented Aug 31, 2022 •

edited

Loading

ti-chi-bot commented Aug 31, 2022 •

edited

Loading

ti-chi-bot commented Aug 31, 2022 •

edited

Loading

zimulala Sep 2, 2022

tangenta Sep 2, 2022

zimulala Sep 2, 2022

tangenta Sep 2, 2022

zimulala Sep 2, 2022

tangenta Sep 2, 2022

zimulala Sep 5, 2022

tangenta Sep 5, 2022

zimulala Sep 5, 2022

tangenta Sep 5, 2022

zimulala Sep 5, 2022

tangenta Sep 5, 2022

tangenta commented Sep 5, 2022

ti-chi-bot commented Sep 5, 2022

tangenta commented Sep 5, 2022

Benjamin2037 left a comment

ti-chi-bot commented Sep 5, 2022

bb7133 left a comment

bb7133 commented Sep 5, 2022

ti-chi-bot commented Sep 5, 2022

bb7133 commented Sep 5, 2022

tangenta commented Sep 6, 2022

sre-bot commented Sep 6, 2022

ddl/ingest: add the management infra for lightning struct #37521

ddl/ingest: add the management infra for lightning struct #37521

Conversation

tangenta commented Aug 31, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Aug 31, 2022 • edited Loading

ti-chi-bot commented Aug 31, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangenta commented Sep 5, 2022

ti-chi-bot commented Sep 5, 2022

tangenta commented Sep 5, 2022

Benjamin2037 left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Sep 5, 2022

bb7133 left a comment

Choose a reason for hiding this comment

bb7133 commented Sep 5, 2022

ti-chi-bot commented Sep 5, 2022

bb7133 commented Sep 5, 2022

tangenta commented Sep 6, 2022

sre-bot commented Sep 6, 2022

TiDB MergeCI notify

tangenta commented Aug 31, 2022 •

edited

Loading

ti-chi-bot commented Aug 31, 2022 •

edited

Loading

ti-chi-bot commented Aug 31, 2022 •

edited

Loading