import/index: adjust concurrency using cpu-count of managed node depends on use case #50091

D3Hunter · 2024-01-04T11:29:08Z

What problem does this PR solve?

Issue Number: ref #49008

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

tiprow · 2024-01-04T11:29:25Z

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.

ywqzzy · 2024-01-04T11:36:34Z

pkg/ddl/index.go

+		if err != nil {
+			return err
+		}


retryable？

it happens before create task, seems the new added errors should be checked manually: no managed node and no managed node have enough resource

ywqzzy · 2024-01-04T11:38:20Z

pkg/executor/importer/import.go

+		return cpu.GetCPUCount(), nil
+	}
+	// the call path of initialization of threadCnt don't have context, so we use a timeout here.
+	ctx, cancel := context.WithTimeout(context.Background(), getCPUCountTimeout)


What if have a heavy workload in CI？

codecov · 2024-01-04T11:40:41Z

Codecov Report

Merging #50091 (d870a10) into master (75aa3c7) will decrease coverage by 14.5850%.
Report is 14 commits behind head on master.
The diff coverage is 86.8421%.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #50091         +/-   ##
=================================================
- Coverage   70.0593%   55.4743%   -14.5850%     
=================================================
  Files          1440       1554        +114     
  Lines        419536     588825     +169289     
=================================================
+ Hits         293924     326647      +32723     
- Misses       105300     239391     +134091     
- Partials      20312      22787       +2475

Flag	Coverage Δ
integration	`36.9388% <76.3157%> (?)`
unit	`70.0280% <54.4117%> (-0.0313%)`	⬇️

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

Components	Coverage Δ
dumpling	`54.0269% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`55.7686% <ø> (+6.0644%)`	⬆️

D3Hunter · 2024-01-04T11:58:20Z

/hold

D3Hunter · 2024-01-04T13:33:07Z

/unhold

D3Hunter · 2024-01-05T02:18:16Z

/retest

tiprow · 2024-01-05T02:18:39Z

@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 · 2024-01-08T03:34:59Z

cc @ywqzzy @tangenta

ywqzzy

Rest LGTM

pkg/disttask/framework/storage/task_table.go

ywqzzy · 2024-01-09T05:58:43Z

pkg/executor/importer/import.go

@@ -141,6 +143,8 @@ var (
 		".zstd", ".zst",
 		".snappy",
 	}
+
+	getCPUCountTimeout = time.Minute


Suggested change

getCPUCountTimeout = time.Minute

ywqzzy · 2024-01-09T05:59:18Z

pkg/executor/importer/importer_testkit_test.go

+		require.NoError(t, failpoint.Disable("github.com/pingcap/tidb/pkg/util/cpu/mockNumCpu"))
+		variable.EnableDistTask.Store(false)
+	})
+	require.NoError(t, tm.StartManager(ctx, "tidb1", ""))


fix confilt

Co-authored-by: EasonBall <592838129@qq.com>

…rt-threadcnt

ti-chi-bot · 2024-01-10T07:22:38Z

[LGTM Timeline notifier]

Timeline:

2024-01-09 05:59:36.464694557 +0000 UTC m=+336566.048948245: ☑️ agreed by ywqzzy.
2024-01-10 07:22:36.993339837 +0000 UTC m=+427946.577593523: ☑️ agreed by tangenta.

D3Hunter · 2024-01-10T07:23:22Z

/approve

ti-chi-bot · 2024-01-10T07:23:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, tangenta, ywqzzy

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~~ [D3Hunter,tangenta]
~~pkg/ddl/OWNERS~~ [tangenta,ywqzzy]
~~pkg/disttask/OWNERS~~ [D3Hunter,tangenta,ywqzzy]
~~pkg/executor/importer/OWNERS~~ [D3Hunter]

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

D3Hunter added 2 commits January 4, 2024 19:13

change

a73b20f

change

3b4fe56

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

D3Hunter mentioned this pull request Jan 4, 2024

resource control inside dist framework #49008

Open

43 tasks

ywqzzy reviewed Jan 4, 2024

View reviewed changes

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 Jan 4, 2024

change

058bb61

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 Jan 4, 2024

D3Hunter requested a review from tangenta January 4, 2024 13:40

ywqzzy approved these changes Jan 9, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jan 9, 2024

D3Hunter and others added 7 commits January 9, 2024 14:01

Update pkg/disttask/framework/storage/task_table.go

71b53cf

Co-authored-by: EasonBall <592838129@qq.com>

fix comment

8886cfc

Merge remote-tracking branch 'upstream/master' into import-threadcnt

958ffe5

Merge branch 'import-threadcnt' of github.com:D3Hunter/tidb into impo…

27f8269

…rt-threadcnt

fix conflict code

09e0050

Merge remote-tracking branch 'upstream/master' into import-threadcnt

1caf064

Merge remote-tracking branch 'upstream/master' into import-threadcnt

d870a10

tangenta approved these changes Jan 10, 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 10, 2024

ti-chi-bot bot added the approved label Jan 10, 2024

ti-chi-bot bot merged commit e15c517 into pingcap:master Jan 10, 2024
22 checks passed

D3Hunter deleted the import-threadcnt branch January 10, 2024 08:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

import/index: adjust concurrency using cpu-count of managed node depends on use case #50091

import/index: adjust concurrency using cpu-count of managed node depends on use case #50091

D3Hunter commented Jan 4, 2024

tiprow bot commented Jan 4, 2024

ywqzzy Jan 4, 2024

D3Hunter Jan 4, 2024

ywqzzy Jan 4, 2024

D3Hunter Jan 4, 2024

codecov bot commented Jan 4, 2024 •

edited

Loading

D3Hunter commented Jan 4, 2024

D3Hunter commented Jan 4, 2024

D3Hunter commented Jan 5, 2024

tiprow bot commented Jan 5, 2024

D3Hunter commented Jan 8, 2024

ywqzzy left a comment

ywqzzy Jan 9, 2024

ywqzzy Jan 9, 2024

ti-chi-bot bot commented Jan 10, 2024

D3Hunter commented Jan 10, 2024

ti-chi-bot bot commented Jan 10, 2024

import/index: adjust concurrency using cpu-count of managed node depends on use case #50091

import/index: adjust concurrency using cpu-count of managed node depends on use case #50091

Conversation

D3Hunter commented Jan 4, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Jan 4, 2024

ywqzzy Jan 4, 2024

Choose a reason for hiding this comment

D3Hunter Jan 4, 2024

Choose a reason for hiding this comment

ywqzzy Jan 4, 2024

Choose a reason for hiding this comment

D3Hunter Jan 4, 2024

Choose a reason for hiding this comment

codecov bot commented Jan 4, 2024 • edited Loading

Codecov Report

D3Hunter commented Jan 4, 2024

D3Hunter commented Jan 4, 2024

D3Hunter commented Jan 5, 2024

tiprow bot commented Jan 5, 2024

D3Hunter commented Jan 8, 2024

ywqzzy left a comment

Choose a reason for hiding this comment

ywqzzy Jan 9, 2024

Choose a reason for hiding this comment

ywqzzy Jan 9, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Jan 10, 2024

[LGTM Timeline notifier]

D3Hunter commented Jan 10, 2024

ti-chi-bot bot commented Jan 10, 2024

codecov bot commented Jan 4, 2024 •

edited

Loading