external engine: support concurrent read and split into smaller data #47510

lance6716 · 2023-10-10T05:32:23Z

What problem does this PR solve?

Issue Number: ref #45719

Problem Summary:

What is changed and how it works?

When the external engine contains less than 1000 files, we can open multiple iterators to concurrently read the data in given key ranges.
When the region size is larger than 2*96MB, generate region jobs per 96MB KV pairs so the large region still have enough concurrency

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

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-10-10T05:32:48Z

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.

lance6716 · 2023-10-10T05:33:03Z

/test pull-br-integration-test

tiprow · 2023-10-10T05:33:33Z

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

In response to this:

/test pull-br-integration-test

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.

codecov · 2023-10-10T05:48:36Z

Codecov Report

Merging #47510 (e2cee99) into master (1b70096) will increase coverage by 2.8696%.
Report is 27 commits behind head on master.
The diff coverage is 83.1168%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #47510        +/-   ##
================================================
+ Coverage   72.1485%   75.0182%   +2.8696%     
================================================
  Files          1353       1395        +42     
  Lines        401168     408042      +6874     
================================================
+ Hits         289437     306106     +16669     
+ Misses        92480      82124     -10356     
- Partials      19251      19812       +561

Flag	Coverage Δ
integration	`45.2152% <15.5844%> (?)`
unit	`72.1517% <83.1168%> (+0.0031%)`	⬆️

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

Components	Coverage Δ
dumpling	`53.9913% <ø> (ø)`
parser	`84.6574% <ø> (-0.0108%)`	⬇️
br	`65.6724% <88.8888%> (+12.4958%)`	⬆️

wjhuang2016 · 2023-10-10T07:03:57Z

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

+) error {
+	// estimate we will open at most 1000 files, so if e.dataFiles is small we can
+	// try to concurrently process ranges.
+	concurrency := int(MergeSortOverlapThreshold) / len(e.dataFiles)


Set a lower/upper limit

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

lance6716 · 2023-10-10T08:18:54Z

/test pull-br-integration-test

tiprow · 2023-10-10T08:19:24Z

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

In response to this:

/test pull-br-integration-test

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.

Benjamin2037

LGTM

ti-chi-bot · 2023-10-11T07:44:21Z

[LGTM Timeline notifier]

Timeline:

2023-10-10 08:17:58.116547982 +0000 UTC m=+1126675.703658126: ☑️ agreed by wjhuang2016.
2023-10-11 07:44:20.532747 +0000 UTC m=+1211058.119857131: ☑️ agreed by Benjamin2037.

Benjamin2037 · 2023-10-11T07:45:15Z

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

-	return errors.Trace(e.iter.Close())
-}
+// Close implements common.Engine.
+func (e *Engine) Close() error { return nil }


Doube confirm, external engine close do not need release any resource, right?

For the rest lgtm.

yes, local engine need to close its pebble DB. And before this PR external engine holds an iterator member that needs to be closed, but in this PR the iterator becomes a local variable that closed in line 144.

lance6716 · 2023-10-11T07:58:35Z

/retest

tiprow · 2023-10-11T07:59:07Z

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

ti-chi-bot · 2023-10-11T08:33:31Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [YuJuncen,tangenta,wjhuang2016]
~~br/OWNERS~~ [YuJuncen]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tiprow · 2023-10-11T08:40:21Z

@lance6716: 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	`e2cee99`	link	true	`/test tiprow_fast_test`

Full PR test history. Your PR dashboard.

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.

lance6716 added 3 commits October 9, 2023 18:07

--wip-- [skip ci]

4d586f8

finish

7f7ac1c

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

fix bazel

5dd4475

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

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 10, 2023

wjhuang2016 reviewed Oct 10, 2023

View reviewed changes

address comment

e2cee99

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

wjhuang2016 approved these changes Oct 10, 2023

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Oct 10, 2023

Benjamin2037 approved these changes Oct 11, 2023

View reviewed changes

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

Benjamin2037 reviewed Oct 11, 2023

View reviewed changes

YuJuncen approved these changes Oct 11, 2023

View reviewed changes

ti-chi-bot bot added the approved label Oct 11, 2023

tangenta approved these changes Oct 11, 2023

View reviewed changes

ti-chi-bot bot merged commit aa99c16 into pingcap:master Oct 11, 2023
25 of 29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

external engine: support concurrent read and split into smaller data #47510

external engine: support concurrent read and split into smaller data #47510

lance6716 commented Oct 10, 2023

tiprow bot commented Oct 10, 2023

lance6716 commented Oct 10, 2023

tiprow bot commented Oct 10, 2023

codecov bot commented Oct 10, 2023 •

edited

Loading

wjhuang2016 Oct 10, 2023

lance6716 Oct 10, 2023

lance6716 commented Oct 10, 2023

tiprow bot commented Oct 10, 2023

Benjamin2037 left a comment

ti-chi-bot bot commented Oct 11, 2023

Benjamin2037 Oct 11, 2023

Benjamin2037 Oct 11, 2023

lance6716 Oct 11, 2023

lance6716 commented Oct 11, 2023

tiprow bot commented Oct 11, 2023

ti-chi-bot bot commented Oct 11, 2023

tiprow bot commented Oct 11, 2023

external engine: support concurrent read and split into smaller data #47510

external engine: support concurrent read and split into smaller data #47510

Conversation

lance6716 commented Oct 10, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

tiprow bot commented Oct 10, 2023

lance6716 commented Oct 10, 2023

tiprow bot commented Oct 10, 2023

codecov bot commented Oct 10, 2023 • edited Loading

Codecov Report

wjhuang2016 Oct 10, 2023

Choose a reason for hiding this comment

lance6716 Oct 10, 2023

Choose a reason for hiding this comment

lance6716 commented Oct 10, 2023

tiprow bot commented Oct 10, 2023

Benjamin2037 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Oct 11, 2023

[LGTM Timeline notifier]

Benjamin2037 Oct 11, 2023

Choose a reason for hiding this comment

Benjamin2037 Oct 11, 2023

Choose a reason for hiding this comment

lance6716 Oct 11, 2023

Choose a reason for hiding this comment

lance6716 commented Oct 11, 2023

tiprow bot commented Oct 11, 2023

ti-chi-bot bot commented Oct 11, 2023

tiprow bot commented Oct 11, 2023

codecov bot commented Oct 10, 2023 •

edited

Loading