lightning: add ExternalEngineConfig to create an external engine #46636

lance6716 · 2023-09-04T11:17:00Z

What problem does this PR solve?

Issue Number: ref #45719

Problem Summary:

What is changed and how it works?

Add ExternalEngineConfig. local backend CloseEngine will use ExternalEngineConfig to create an external engine, and then local backend can import it with the UUID.

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

Signed-off-by: lance6716 <lance6716@gmail.com>

tiprow · 2023-09-04T11:17:24Z

Hi @lance6716. 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.

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.

Signed-off-by: lance6716 <lance6716@gmail.com>

codecov · 2023-09-05T07:08:35Z

Codecov Report

Merging #46636 (820326d) into master (777e6da) will decrease coverage by 0.5925%.
Report is 4 commits behind head on master.
The diff coverage is 60.7476%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #46636        +/-   ##
================================================
- Coverage   73.2843%   72.6918%   -0.5925%     
================================================
  Files          1319       1340        +21     
  Lines        395850     402517      +6667     
================================================
+ Hits         290096     292597      +2501     
- Misses        87263      91387      +4124     
- Partials      18491      18533        +42

Flag	Coverage Δ
integration	`27.8551% <0.0000%> (?)`
unit	`73.3323% <63.1068%> (+0.0480%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`54.0444% <ø> (ø)`
parser	`85.0414% <ø> (+0.0869%)`	⬆️
br	`48.1355% <60.7476%> (-4.3300%)`	⬇️

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2023-09-05T08:34:44Z

br/pkg/lightning/backend/local/local_test.go

+		}
+	}()
+	go func() {
+		err2 := local.CloseEngine(


this is the expected usage

Signed-off-by: lance6716 <lance6716@gmail.com>

br/pkg/lightning/backend/local/local.go

tangenta

Rest LGTM

Signed-off-by: lance6716 <lance6716@gmail.com>

wjhuang2016 · 2023-09-06T06:15:45Z

br/pkg/lightning/backend/external/engine.go

@@ -177,13 +185,13 @@ func (e *Engine) createMergeIter(ctx context.Context, start kv.Key) (*MergeKVIte
 	return iter, nil
 }

-// KVStatistics returns the total kv size and total kv length.
+// KVStatistics returns the total kv size and total kv count.
 func (e *Engine) KVStatistics() (totalKVSize int64, totalKVLength int64) {


Suggested change

func (e *Engine) KVStatistics() (totalKVSize int64, totalKVLength int64) {

func (e *Engine) KVStatistics() (totalKVSize int64, totalKVCount int64) {

wjhuang2016 · 2023-09-06T06:18:16Z

br/pkg/lightning/backend/local/local.go

-	allEngines := local.lockAllEnginesUnless(importMutexStateClose, 0)
+	allEngines := make([]common.Engine, 0, 8)
+	for _, e := range local.externalEngine {
+		allEngines = append(allEngines, e)


Why not close it directly?

wjhuang2016 · 2023-09-06T06:23:52Z

br/pkg/storage/local.go

-	file, err := os.Create(filepath.Join(l.base, name))
+	filename := filepath.Join(l.base, name)
+	dir := filepath.Dir(filename)
+	err := os.MkdirAll(dir, 0750)


Why do you change it?

in other storage backend like s3, name can contain /. But LocalStorage will raise error. I found it when using LocalStorage in the unit tests of this PR

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot · 2023-09-06T07:16:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tangenta, wjhuang2016

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~~ [tangenta,wjhuang2016]

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-06T07:16:13Z

[LGTM Timeline notifier]

Timeline:

2023-09-05 09:13:08.276384633 +0000 UTC m=+2436752.825400619: ☑️ agreed by tangenta.
2023-09-06 07:16:12.684356247 +0000 UTC m=+2516137.233372228: ☑️ agreed by wjhuang2016.

tangenta · 2023-09-06T19:05:48Z

/retest

tiprow · 2023-09-06T19:06:24Z

@tangenta: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2023-09-07T07:27:40Z

/retest

tiprow · 2023-09-07T07:28:12Z

@lance6716: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

lance6716 · 2023-09-07T08:33:50Z

/retest

tiprow · 2023-09-07T08:34:21Z

@lance6716: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

wjhuang2016 · 2023-09-07T12:18:41Z

/retest

lightning: add ExternalEngineConfig to create an external engine

b82492e

Signed-off-by: lance6716 <lance6716@gmail.com>

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 Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 4, 2023

lance6716 added 3 commits September 4, 2023 19:18

add two more config

ba62aee

Signed-off-by: lance6716 <lance6716@gmail.com>

fix UT

fc7e903

Signed-off-by: lance6716 <lance6716@gmail.com>

fix bazel

3a5da7a

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 added 2 commits September 5, 2023 15:12

fix lint

1ede83e

Signed-off-by: lance6716 <lance6716@gmail.com>

add UT

0c60d4c

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 5, 2023

lance6716 changed the title ~~[WIP]lightning: add ExternalEngineConfig to create an external engine~~ lightning: add ExternalEngineConfig to create an external engine Sep 5, 2023

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 Sep 5, 2023

lance6716 commented Sep 5, 2023

View reviewed changes

br/pkg/lightning/backend/local/local_test.go

}

}()

go func() {

err2 := local.CloseEngine(

Copy link

Contributor Author

lance6716 Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the expected usage

lance6716 added 2 commits September 5, 2023 16:38

fix sec lint

faae105

Signed-off-by: lance6716 <lance6716@gmail.com>

fix bazel

549d0da

Signed-off-by: lance6716 <lance6716@gmail.com>

tangenta reviewed Sep 5, 2023

View reviewed changes

br/pkg/lightning/backend/local/local.go Outdated Show resolved Hide resolved

tangenta reviewed Sep 5, 2023

View reviewed changes

tangenta approved these changes Sep 5, 2023

View reviewed changes

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

lance6716 added 3 commits September 5, 2023 18:07

address comment

2c6257b

Signed-off-by: lance6716 <lance6716@gmail.com>

have to use failpoint

ea7494d

Signed-off-by: lance6716 <lance6716@gmail.com>

fix CI

7f7bef5

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot bot added the do-not-merge/needs-tests-checked label Sep 5, 2023

wuhuizuo removed the do-not-merge/needs-tests-checked label Sep 6, 2023

wjhuang2016 reviewed Sep 6, 2023

View reviewed changes

address comment

4a89257

Signed-off-by: lance6716 <lance6716@gmail.com>

wjhuang2016 approved these changes Sep 6, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 6, 2023

lance6716 added 2 commits September 7, 2023 11:18

switch to failpoint

f6de222

Signed-off-by: lance6716 <lance6716@gmail.com>

Merge branch 'master' of github.com:pingcap/tidb into external-config

820326d

tangenta mentioned this pull request Sep 7, 2023

ddl, lightning: integrate operator for global sort #46734

Merged

12 tasks

ti-chi-bot bot merged commit e01d179 into pingcap:master Sep 7, 2023
17 of 21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lightning: add ExternalEngineConfig to create an external engine #46636

lightning: add ExternalEngineConfig to create an external engine #46636

lance6716 commented Sep 4, 2023 •

edited

Loading

tiprow bot commented Sep 4, 2023

codecov bot commented Sep 5, 2023 •

edited

Loading

lance6716 Sep 5, 2023

tangenta left a comment

wjhuang2016 Sep 6, 2023

wjhuang2016 Sep 6, 2023

lance6716 Sep 6, 2023

wjhuang2016 Sep 6, 2023

lance6716 Sep 6, 2023

ti-chi-bot bot commented Sep 6, 2023

ti-chi-bot bot commented Sep 6, 2023

tangenta commented Sep 6, 2023

tiprow bot commented Sep 6, 2023

lance6716 commented Sep 7, 2023

tiprow bot commented Sep 7, 2023

lance6716 commented Sep 7, 2023

tiprow bot commented Sep 7, 2023

wjhuang2016 commented Sep 7, 2023

	func (e *Engine) KVStatistics() (totalKVSize int64, totalKVLength int64) {
	func (e *Engine) KVStatistics() (totalKVSize int64, totalKVCount int64) {

lightning: add ExternalEngineConfig to create an external engine #46636

lightning: add ExternalEngineConfig to create an external engine #46636

Conversation

lance6716 commented Sep 4, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

tiprow bot commented Sep 4, 2023

codecov bot commented Sep 5, 2023 • edited Loading

Codecov Report

lance6716 Sep 5, 2023

Choose a reason for hiding this comment

tangenta left a comment

Choose a reason for hiding this comment

wjhuang2016 Sep 6, 2023

Choose a reason for hiding this comment

wjhuang2016 Sep 6, 2023

Choose a reason for hiding this comment

lance6716 Sep 6, 2023

Choose a reason for hiding this comment

wjhuang2016 Sep 6, 2023

Choose a reason for hiding this comment

lance6716 Sep 6, 2023

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 6, 2023

ti-chi-bot bot commented Sep 6, 2023

[LGTM Timeline notifier]

tangenta commented Sep 6, 2023

tiprow bot commented Sep 6, 2023

lance6716 commented Sep 7, 2023

tiprow bot commented Sep 7, 2023

lance6716 commented Sep 7, 2023

tiprow bot commented Sep 7, 2023

wjhuang2016 commented Sep 7, 2023

lance6716 commented Sep 4, 2023 •

edited

Loading

codecov bot commented Sep 5, 2023 •

edited

Loading