importinto: executor part of import from select #50341

D3Hunter · 2024-01-11T11:08:57Z

What problem does this PR solve?

Issue Number: ref #49883

Problem Summary:

What changed and how does it work?

see title

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

tiprow · 2024-01-11T11:09:14Z

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 · 2024-01-11T11:12:34Z

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

@@ -517,24 +516,6 @@ func NewBackend(
 		return nil, common.NormalizeOrWrapErr(common.ErrCreatePDClient, err)
 	}

-	shouldCreate := true


moved to engine-mgr, below Close part too

D3Hunter · 2024-01-11T11:12:57Z

/test pull-lightning-integration-test

tiprow · 2024-01-11T11:13:20Z

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

In response to this:

/test pull-lightning-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 · 2024-01-11T11:28:21Z

Codecov Report

Merging #50341 (6c1c867) into master (cfe245d) will decrease coverage by 14.5912%.
Report is 70 commits behind head on master.
The diff coverage is 76.4091%.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #50341         +/-   ##
=================================================
- Coverage   70.0461%   55.4550%   -14.5912%     
=================================================
  Files          1444       1565        +121     
  Lines        419849     592639     +172790     
=================================================
+ Hits         294088     328648      +34560     
- Misses       105440     241115     +135675     
- Partials      20321      22876       +2555

Flag	Coverage Δ
integration	`36.6811% <67.4321%> (?)`
unit	`70.1937% <55.9748%> (+0.1475%)`	⬆️

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

Components	Coverage Δ
dumpling	`54.0269% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`56.5468% <48.8888%> (+6.8623%)`	⬆️

D3Hunter · 2024-01-12T02:19:20Z

ci success
https://do.pingcap.net/jenkins/blue/organizations/jenkins/pingcap%2Ftidb%2Fpull_lightning_integration_test/detail/pull_lightning_integration_test/23/pipeline

D3Hunter · 2024-01-15T10:44:22Z

@lance6716 @GMHDBJD

lance6716

7 / 25 files viewed

lance6716 · 2024-01-16T07:34:30Z

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

+	}
+
+	if shouldCreate {
+		err := os.Mkdir(config.LocalStoreDir, 0o700)


use MkdirAll?

copied from local backend ctor

pkg/executor/import_into.go

lance6716 · 2024-01-16T08:15:54Z

pkg/executor/import_into.go

+				select {
+				case selectedRowCh <- importer.QueryRow{
+					ID:   idAllocator,
+					Data: innerChunkRow.GetDatumRow(fields),


if we can send a whole chk to ImportSelectedRows, after chk is finished reading we can put it back to pool

the memory footprint is not that large here, let gc handle it now.

lance6716

11 / 25 files viewed

lance6716 · 2024-01-17T03:42:33Z

pkg/executor/importer/chunk_process.go

+type chunkEncoder interface {
+	init() error
+	encodeLoop(ctx context.Context) error
+	logFields() []zap.Field


seems it's only used when task ends. In order to not let developer use it when task is running, I prefer we name it summary, summaryFields or something.

lance6716 · 2024-01-17T03:46:15Z

pkg/executor/importer/chunk_process.go

+	return nil
+}
+
+func (e *queryChunkEncoder) encodeLoop(ctx context.Context) error {


maybe in future we can unify it with fileChunkEncoder? Most logic is shared, the difference is how to read data.

6c1c867 add a todo

lance6716

rest lgtm

GMHDBJD

LGTM

ti-chi-bot · 2024-01-17T10:47:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GMHDBJD, lance6716

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~~ [GMHDBJD,lance6716]
~~br/pkg/lightning/OWNERS~~ [GMHDBJD,lance6716]
~~pkg/executor/importer/OWNERS~~ [GMHDBJD,lance6716]

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 · 2024-01-17T10:47:30Z

[LGTM Timeline notifier]

Timeline:

2024-01-17 09:47:21.571179372 +0000 UTC m=+349283.135477061: ☑️ agreed by lance6716.
2024-01-17 10:47:28.475690137 +0000 UTC m=+352890.039987839: ☑️ agreed by GMHDBJD.

D3Hunter · 2024-01-17T14:59:38Z

/retest

tiprow · 2024-01-17T15:00:02Z

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

D3Hunter added 4 commits January 11, 2024 17:25

change

d82cc9a

i t

0c8051f

test

4ddb22d

bazel

84aa013

ti-chi-bot 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 Jan 11, 2024

D3Hunter requested review from lance6716 and GMHDBJD January 11, 2024 11:09

D3Hunter mentioned this pull request Jan 11, 2024

import into from select #49883

Closed

7 tasks

change

f79fd03

D3Hunter commented Jan 11, 2024

View reviewed changes

D3Hunter added 2 commits January 12, 2024 11:01

more test

7659774

fix test

f3748a6

lance6716 reviewed Jan 16, 2024

View reviewed changes

add comments

87c73b0

lance6716 reviewed Jan 17, 2024

View reviewed changes

fix comments

6c1c867

lance6716 approved these changes Jan 17, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jan 17, 2024

GMHDBJD approved these changes Jan 17, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 17, 2024

ti-chi-bot bot merged commit 12d273e into pingcap:master Jan 17, 2024
28 checks passed

D3Hunter deleted the imp-from-sel branch January 17, 2024 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

importinto: executor part of import from select #50341

importinto: executor part of import from select #50341

D3Hunter commented Jan 11, 2024

tiprow bot commented Jan 11, 2024

D3Hunter Jan 11, 2024

D3Hunter commented Jan 11, 2024

tiprow bot commented Jan 11, 2024

codecov bot commented Jan 11, 2024 •

edited

Loading

D3Hunter commented Jan 12, 2024

D3Hunter commented Jan 15, 2024

lance6716 left a comment

lance6716 Jan 16, 2024

D3Hunter Jan 16, 2024

lance6716 Jan 16, 2024

D3Hunter Jan 16, 2024

lance6716 left a comment

lance6716 Jan 17, 2024

lance6716 Jan 17, 2024

D3Hunter Jan 17, 2024 •

edited

Loading

lance6716 left a comment

GMHDBJD left a comment

ti-chi-bot bot commented Jan 17, 2024

ti-chi-bot bot commented Jan 17, 2024

D3Hunter commented Jan 17, 2024

tiprow bot commented Jan 17, 2024

importinto: executor part of import from select #50341

importinto: executor part of import from select #50341

Conversation

D3Hunter commented Jan 11, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Jan 11, 2024

D3Hunter Jan 11, 2024

Choose a reason for hiding this comment

D3Hunter commented Jan 11, 2024

tiprow bot commented Jan 11, 2024

codecov bot commented Jan 11, 2024 • edited Loading

Codecov Report

D3Hunter commented Jan 12, 2024

D3Hunter commented Jan 15, 2024

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 Jan 16, 2024

Choose a reason for hiding this comment

D3Hunter Jan 16, 2024

Choose a reason for hiding this comment

lance6716 Jan 16, 2024

Choose a reason for hiding this comment

D3Hunter Jan 16, 2024

Choose a reason for hiding this comment

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 Jan 17, 2024

Choose a reason for hiding this comment

lance6716 Jan 17, 2024

Choose a reason for hiding this comment

D3Hunter Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

lance6716 left a comment

Choose a reason for hiding this comment

GMHDBJD left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jan 17, 2024

ti-chi-bot bot commented Jan 17, 2024

[LGTM Timeline notifier]

D3Hunter commented Jan 17, 2024

tiprow bot commented Jan 17, 2024

codecov bot commented Jan 11, 2024 •

edited

Loading

D3Hunter Jan 17, 2024 •

edited

Loading