importinto: use one writer for each kv group for all concurrent encoder #47185

D3Hunter · 2023-09-22T04:21:17Z

What problem does this PR solve?

Issue Number: ref #46704

Problem Summary:

in current design of global sort, each kv group, such as data kv or kv of same index id, should uses 1 writer

importinto change to it too

What is changed and how it works?

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

ti-chi-bot · 2023-09-22T04:21:27Z

Hi @D3Hunter. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

tiprow · 2023-09-22T04:21:43Z

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

D3Hunter · 2023-09-22T04:22:13Z

/ok-to-test

ti-chi-bot · 2023-09-22T04:22:29Z

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

In response to this:

/ok-to-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.

D3Hunter · 2023-09-22T04:22:45Z

/label ok-to-test

tiprow · 2023-09-22T04:22:46Z

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

In response to this:

/ok-to-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.

D3Hunter · 2023-09-22T04:23:05Z

/remove-label needs-ok-to-test

wuhuizuo · 2023-09-22T04:32:39Z

/test all

codecov · 2023-09-22T04:43:31Z

Codecov Report

Merging #47185 (145542b) into master (7a116f3) will decrease coverage by 0.3016%.
Report is 2 commits behind head on master.
The diff coverage is 73.2673%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #47185        +/-   ##
================================================
- Coverage   72.9737%   72.6722%   -0.3016%     
================================================
  Files          1340       1361        +21     
  Lines        399989     406308      +6319     
================================================
+ Hits         291887     295273      +3386     
- Misses        89206      92260      +3054     
+ Partials      18896      18775       -121

Flag	Coverage Δ
integration	`32.3259% <4.9505%> (?)`
unit	`72.9807% <73.2673%> (+0.0069%)`	⬆️

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

Components	Coverage Δ
dumpling	`53.9913% <ø> (ø)`
parser	`84.9365% <ø> (ø)`
br	`48.6979% <100.0000%> (-4.3420%)`	⬇️

Benjamin2037 · 2023-09-22T05:07:54Z

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

@@ -488,6 +488,9 @@ func (w *Writer) createStorageWriter(ctx context.Context) (

 // EngineWriter implements backend.EngineWriter interface.
 type EngineWriter struct {
+	// Only 1 writer is used for some kv group(data or some index), no matter
+	// how many routines are encoding data, so need to sync write to it.
+	sync.Mutex


Why here add new Mutex，Could we reuse，global sort mutex that wenjun introduced in #47131

see comments, i think that's quite clear

global sort mutex that wenjun introduced in #47131

that's shared among all writers

Benjamin2037 · 2023-09-22T05:29:45Z

/assign windtalker

Benjamin2037 · 2023-09-22T05:30:25Z

/assign @windtalker

lance6716 · 2023-09-22T05:56:44Z

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

@@ -94,6 +95,10 @@ func newByteReader(

 // switchReaderMode switches to concurrent reader.
 func (r *byteReader) switchReaderMode(useConcurrent bool) {
+	failpoint.Inject("disableConcurrentReader", func() {
+		// current impl only supports S3, but we need test using GCS
+		useConcurrent = false


Benjamin2037

LGTM

lance6716 · 2023-09-22T06:20:46Z

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

@@ -488,6 +488,9 @@ func (w *Writer) createStorageWriter(ctx context.Context) (

 // EngineWriter implements backend.EngineWriter interface.


maybe mention it's concurrent safe in comments

lance6716 · 2023-09-22T06:32:52Z

executor/importer/chunk_process.go

-			writer = w.writerFactory(indexID)
-			w.writers[indexID] = writer
-		}
+		writer := w.getWriter(indexID)
 		if err = writer.WriteRow(ctx, item.Key, item.Val, nil); err != nil {


seems if one writer has entered WriteRow, it will block other kvs which also need to access this writer. We can improve it later.

do you mean kvs in this loop? or in other concurrency?

other concurrent call of WriteRow. I expect the idle writer can still do something

depends on whether it's same index, other concurrency can still use writers of other index

ti-chi-bot · 2023-09-22T06:33:07Z

[LGTM Timeline notifier]

Timeline:

2023-09-22 06:01:05.813523199 +0000 UTC m=+842831.781111247: ☑️ agreed by Benjamin2037.
2023-09-22 06:33:06.82576401 +0000 UTC m=+844752.793352060: ☑️ agreed by lance6716.

D3Hunter · 2023-09-22T06:46:01Z

/hold

D3Hunter · 2023-09-22T06:49:42Z

need wait #47170 merged

windtalker

LGTM

ti-chi-bot · 2023-09-22T06:56:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Benjamin2037, lance6716, windtalker

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~~ [lance6716,windtalker]
~~executor/OWNERS~~ [windtalker]

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

D3Hunter · 2023-09-22T10:23:21Z

/retest

D3Hunter · 2023-09-22T10:54:36Z

/unhold

…nt encoder (pingcap#47185)" This reverts commit 4e82952.

D3Hunter added 6 commits September 21, 2023 22:14

change

5197dd1

bazel

f0b3a91

change

2e6c658

test

34ba8e5

test

fde055e

Merge remote-tracking branch 'upstream/master' into use-one-writer

2d79f28

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. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Sep 22, 2023

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Sep 22, 2023

ti-chi-bot bot removed the needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. label Sep 22, 2023

Benjamin2037 reviewed Sep 22, 2023

View reviewed changes

ti-chi-bot bot assigned windtalker Sep 22, 2023

lint

9d954f6

lance6716 reviewed Sep 22, 2023

View reviewed changes

Benjamin2037 approved these changes Sep 22, 2023

View reviewed changes

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

lance6716 approved these changes Sep 22, 2023

View reviewed changes

ti-chi-bot bot added the lgtm label Sep 22, 2023

ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Sep 22, 2023

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

windtalker approved these changes Sep 22, 2023

View reviewed changes

ti-chi-bot bot added the approved label Sep 22, 2023

D3Hunter added 3 commits September 22, 2023 15:02

fix test, remove failpoint

58958f8

Merge remote-tracking branch 'upstream/master' into use-one-writer

b884bdb

bazel

145542b

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

ti-chi-bot bot merged commit 4e82952 into pingcap:master Sep 22, 2023

D3Hunter deleted the use-one-writer branch September 22, 2023 11:03

D3Hunter added a commit to D3Hunter/tidb that referenced this pull request Oct 12, 2023

Revert "importinto: use one writer for each kv group for all concurre…

2f4f8f3

…nt encoder (pingcap#47185)" This reverts commit 4e82952.

D3Hunter added a commit to D3Hunter/tidb that referenced this pull request Oct 16, 2023

Revert "importinto: use one writer for each kv group for all concurre…

5ed8c9b

…nt encoder (pingcap#47185)" This reverts commit 4e82952.

D3Hunter mentioned this pull request Oct 16, 2023

importinto: revert "use one writer for each kv group for all concurrent encoder" #47666

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

importinto: use one writer for each kv group for all concurrent encoder #47185

importinto: use one writer for each kv group for all concurrent encoder #47185

D3Hunter commented Sep 22, 2023

ti-chi-bot bot commented Sep 22, 2023

tiprow bot commented Sep 22, 2023

D3Hunter commented Sep 22, 2023

ti-chi-bot bot commented Sep 22, 2023

D3Hunter commented Sep 22, 2023

tiprow bot commented Sep 22, 2023

D3Hunter commented Sep 22, 2023

wuhuizuo commented Sep 22, 2023

codecov bot commented Sep 22, 2023 •

edited

Loading

Benjamin2037 Sep 22, 2023

D3Hunter Sep 22, 2023

Benjamin2037 commented Sep 22, 2023

Benjamin2037 commented Sep 22, 2023

lance6716 Sep 22, 2023

Benjamin2037 left a comment

lance6716 Sep 22, 2023

D3Hunter Sep 22, 2023

lance6716 Sep 22, 2023

D3Hunter Sep 22, 2023

lance6716 Sep 22, 2023 •

edited

Loading

D3Hunter Sep 22, 2023

ti-chi-bot bot commented Sep 22, 2023

D3Hunter commented Sep 22, 2023

D3Hunter commented Sep 22, 2023

windtalker left a comment

ti-chi-bot bot commented Sep 22, 2023

D3Hunter commented Sep 22, 2023

D3Hunter commented Sep 22, 2023

		@@ -488,6 +488,9 @@ func (w *Writer) createStorageWriter(ctx context.Context) (

		// EngineWriter implements backend.EngineWriter interface.

importinto: use one writer for each kv group for all concurrent encoder #47185

importinto: use one writer for each kv group for all concurrent encoder #47185

Conversation

D3Hunter commented Sep 22, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot bot commented Sep 22, 2023

tiprow bot commented Sep 22, 2023

D3Hunter commented Sep 22, 2023

ti-chi-bot bot commented Sep 22, 2023

D3Hunter commented Sep 22, 2023

tiprow bot commented Sep 22, 2023

D3Hunter commented Sep 22, 2023

wuhuizuo commented Sep 22, 2023

codecov bot commented Sep 22, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benjamin2037 commented Sep 22, 2023

Benjamin2037 commented Sep 22, 2023

Choose a reason for hiding this comment

Benjamin2037 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

lance6716 Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 22, 2023

[LGTM Timeline notifier]

D3Hunter commented Sep 22, 2023

D3Hunter commented Sep 22, 2023

windtalker left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 22, 2023

D3Hunter commented Sep 22, 2023

D3Hunter commented Sep 22, 2023

codecov bot commented Sep 22, 2023 •

edited

Loading

lance6716 Sep 22, 2023 •

edited

Loading