[DNM]global sort: new merge step #49212

ywqzzy · 2023-12-06T04:50:03Z

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

What changed and how does it work?

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-12-06T04:50:06Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tiprow · 2023-12-06T04:50:41Z

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

…ge_step

codecov · 2023-12-07T11:26:47Z

Codecov Report

Merging #49212 (7b6ab3c) into master (b478056) will decrease coverage by 0.0087%.
Report is 4 commits behind head on master.
The diff coverage is n/a.

❗ Current head 7b6ab3c differs from pull request most recent head e6f7ed5. Consider uploading reports for the commit e6f7ed5 to get more accurate results

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #49212        +/-   ##
================================================
- Coverage   71.0493%   71.0406%   -0.0088%     
================================================
  Files          1368       1368                
  Lines        401715     402727      +1012     
================================================
+ Hits         285416     286100       +684     
- Misses        96443      96696       +253     
- Partials      19856      19931        +75

Flag	Coverage Δ
integration	`∅ <ø> (?)`

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

Components	Coverage Δ
dumpling	`53.9663% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`52.9106% <ø> (+0.0172%)`	⬆️

ti-chi-bot · 2023-12-12T02:53:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign benjamin2037, leavrth for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

…ge_step

D3Hunter · 2023-12-12T07:07:05Z

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

@@ -271,7 +271,7 @@ func (r *byteReader) reload() error {
 	now := r.concurrentReader.now
 	// in read only false -> true is possible
 	if !now && to {
-		r.logger.Info("switch reader mode", zap.Bool("use concurrent mode", true))
+		// r.logger.Info("switch reader mode", zap.Bool("use concurrent mode", true))


change to debug?

D3Hunter · 2023-12-12T07:10:37Z

br/pkg/storage/gcs_extra.go

+	MPUUploadIDQuery   = "uploadId"
+
+	defaultChunkSize       = 5 * 1024 * 1024 // 5 MB
+	defaultRetry           = 3               // 建议默认3~5次，不要太大，否则将可能会导致成为僵尸任务。


translate comments

D3Hunter · 2023-12-12T07:26:47Z

br/pkg/storage/gcs_extra.go

+func (w *GCSWriter) Close() error {
+	if w.currentWorker != nil {
+		// uploads the last part, which is not a full part
+		w.currentWorker.uploadPartAsync()


D3Hunter · 2023-12-12T07:43:28Z

br/pkg/storage/gcs_extra.go

+		return fmt.Errorf("failed to upload part %v too many times, err: %w", uw.index, uw.err)
+	}
+	go func() {
+		err := uw.uploadPart(uw.currentPartNumber, uw.buffer[:uw.offset]) // timeout in few seconds


if we retry upload after some Write, the part-num is changed

ti-chi-bot · 2023-12-12T09:35:27Z

[FORMAT CHECKER NOTIFICATION]

Notice: To remove the do-not-merge/needs-linked-issue label, please provide the linked issue number on one line in the PR body, for example: Issue Number: close #123 or Issue Number: ref #456.

_{📖 For more info, you can check the "Contribute Code" section in the development guide.}

Notice: To remove the do-not-merge/needs-tests-checked label, please finished the tests then check the finished items in description.

For example:

Tests

Unit test

Integration test

Manual test (add detailed scripts or steps below)

No code

_{📖 For more info, you can check the "Contribute Code" section in the development guide.}

D3Hunter · 2023-12-18T06:09:02Z

will review again when #48939 merged

tangenta · 2023-12-18T06:36:57Z

pkg/disttask/importinto/planner.go

+	return false
+	// failpoint.Inject("forceMergeSort", func(val failpoint.Value) {
+	// 	in := val.(string)
+	// 	if in == kvGroup || in == "*" {
+	// 		failpoint.Return(false)
+	// 	}
+	// })
+	// return external.GetMaxOverlappingTotal(stats) <= external.MergeSortOverlapThreshold


Why change this?

ti-chi-bot · 2023-12-19T12:07:24Z

@ywqzzy: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
idc-jenkins-ci-tidb/build	`e6f7ed5`	link	true	`/test build`
idc-jenkins-ci-tidb/check_dev	`e6f7ed5`	link	true	`/test check-dev`
idc-jenkins-ci-tidb/unit-test	`e6f7ed5`	link	true	`/test unit-test`
idc-jenkins-ci-tidb/check_dev_2	`e6f7ed5`	link	true	`/test check-dev2`
pull-integration-ddl-test	`e6f7ed5`	link	true	`/test pull-integration-ddl-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.

prepare basic work

6618963

ywqzzy added 2 commits December 6, 2023 12:52

add todo

a64fc2a

some update

7ed7a88

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 6, 2023

ywqzzy added 14 commits December 6, 2023 15:20

fix

95c7eb3

track stat

ce1bb2d

revert change

7af2575

plan

fa51860

fix

e4cbbd1

revert

276b7c8

Merge branch 'master' of https://github.com/pingcap/tidb into new_mer…

e4abf86

…ge_step

revert

376911e

some update, with bug

ee887e0

u

b3bda33

test passed

ea80324

fix

27ba338

revert

71bfe55

u

7b6ab3c

ywqzzy marked this pull request as ready for review December 7, 2023 11:15

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 Dec 7, 2023

change

4f83ccf

ywqzzy added 5 commits December 11, 2023 14:13

u

9d4b7cc

pass all tests

2b5e0b1

multi upload

a2d16ea

tmp save

2e81eb0

fix build

7874a21

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2023

ywqzzy added 4 commits December 12, 2023 12:02

tmp save

d1bfe95

remove useless code

6ecd688

Merge branch 'master' of https://github.com/pingcap/tidb into new_mer…

447e4a8

…ge_step

more bench

a1a9560

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2023

fix

6d674ef

D3Hunter reviewed Dec 12, 2023

View reviewed changes

ywqzzy added 2 commits December 12, 2023 16:13

u

2a93126

tmp save

e6f7ed5

D3Hunter mentioned this pull request Dec 14, 2023

storage: support parallel write for gcs #48939

Closed

13 tasks

tangenta reviewed Dec 18, 2023

View reviewed changes

ywqzzy closed this Dec 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DNM]global sort: new merge step #49212

[DNM]global sort: new merge step #49212

ywqzzy commented Dec 6, 2023

ti-chi-bot bot commented Dec 6, 2023

tiprow bot commented Dec 6, 2023

codecov bot commented Dec 7, 2023 •

edited

Loading

ti-chi-bot bot commented Dec 12, 2023

D3Hunter Dec 12, 2023

D3Hunter Dec 12, 2023

D3Hunter Dec 12, 2023

D3Hunter Dec 12, 2023

ti-chi-bot bot commented Dec 12, 2023

D3Hunter commented Dec 18, 2023

tangenta Dec 18, 2023

ti-chi-bot bot commented Dec 19, 2023

[DNM]global sort: new merge step #49212

[DNM]global sort: new merge step #49212

Conversation

ywqzzy commented Dec 6, 2023

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Dec 6, 2023

tiprow bot commented Dec 6, 2023

codecov bot commented Dec 7, 2023 • edited Loading

Codecov Report

ti-chi-bot bot commented Dec 12, 2023

D3Hunter Dec 12, 2023

Choose a reason for hiding this comment

D3Hunter Dec 12, 2023

Choose a reason for hiding this comment

D3Hunter Dec 12, 2023

Choose a reason for hiding this comment

D3Hunter Dec 12, 2023

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 12, 2023

D3Hunter commented Dec 18, 2023

tangenta Dec 18, 2023

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 19, 2023

codecov bot commented Dec 7, 2023 •

edited

Loading