disttask: collect metering data to subtask's real time summary by tangenta · Pull Request #63431 · pingcap/tidb

tangenta · 2025-09-09T16:27:08Z

What problem does this PR solve?

Issue Number: ref #61702

Problem Summary:

What changed and how does it work?

Since DXF subtasks are executed by system keyspace TiDB instances that scaled out automatically, we need to meter the resource usage for billing. This PR collects four metrics for adding index using DXF :

Table scan data network traffic.
Actual index data size.
S3 put request count.
S3 get request count.

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: tangenta <tangenta@126.com>

tiprow · 2025-09-09T16:27:24Z

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

Details

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-sigs/prow repository.

tangenta · 2025-09-09T16:33:05Z

+	tools/bin/mockgen -package mock github.com/pingcap/tidb/pkg/lightning/backend Backend,EngineWriter,TargetInfoGetter > br/pkg/mock/backend.go
+	tools/bin/mockgen -package mock github.com/pingcap/tidb/pkg/lightning/common ChunkFlushStatus > br/pkg/mock/common.go


ChunkFlushStatus is moved from package backend to common to avoid package circular dependency between external and backend.

This PR adds the dependency backend --(depends on)-> external because I added OnReaderClose to the external engine configuration, which is used to collect reading related information.

tangenta · 2025-09-09T16:34:19Z

 	github.com/stretchr/testify v1.10.0
 	github.com/tiancaiamao/appdash v0.0.0-20181126055449-889f96f722a2
-	github.com/tikv/client-go/v2 v2.0.8-0.20250828075934-b794d681774f
+	github.com/tikv/client-go/v2 v2.0.8-0.20250905073636-469a7adf7ae8


Update client-go for bug-fix tikv/client-go#1752

Signed-off-by: tangenta <tangenta@126.com>

codecov · 2025-09-09T16:56:24Z

Codecov Report

❌ Patch coverage is 62.31884% with 78 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.8373%. Comparing base (68d18d6) to head (64b75f4).
⚠️ Report is 34 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #63431        +/-   ##
================================================
+ Coverage   72.7588%   74.8373%   +2.0784%     
================================================
  Files          1835       1886        +51     
  Lines        496690     512660     +15970     
================================================
+ Hits         361386     383661     +22275     
+ Misses       113324     105409      -7915     
- Partials      21980      23590      +1610

Flag	Coverage Δ
integration	`48.5177% <2.4154%> (?)`
unit	`72.3531% <62.0689%> (+0.0657%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.8700% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`63.3236% <56.2500%> (+16.8500%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

D3Hunter · 2025-09-10T02:41:50Z

/retest

tiprow · 2025-09-10T02:42:15Z

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

Details

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-sigs/prow repository.

Signed-off-by: tangenta <tangenta@126.com>

joechenrh · 2025-09-10T04:00:36Z

 		if err != nil {
 			return "", "", "", err
 		}
+		w.putReqCnt++


Does the write count equals to "S3 put request count", since we use a buffered writer.

tidb/br/pkg/storage/s3.go

Lines 1232 to 1237 in d022959

bufSize := WriteBufferSize

if option != nil && option.PartSize > 0 {

bufSize = int(option.PartSize)

}

uploaderWriter := newBufferedWriter(uploader, bufSize, NoCompression)

return uploaderWriter, nil

and we are using multi part upload, does this counted as 1 PUT, or each part is one PUT?

tangenta · 2025-09-10T06:42:31Z

/hold

Signed-off-by: tangenta <tangenta@126.com>

D3Hunter

29/65

Signed-off-by: tangenta <tangenta@126.com>

D3Hunter · 2025-09-11T05:22:17Z

/retest

tiprow · 2025-09-11T05:22:42Z

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

Details

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-sigs/prow repository.

D3Hunter

34/65

D3Hunter · 2025-09-11T06:30:32Z

+	if option != nil {
+		onFlush = option.OnUpload
+		if onFlush != nil {
+			onFlush() // Initiate: PUT


can you also paste the link to how AWS cal API count of the multi part upload in the comment?

check whether we are using multi-part-upload?

check whether we are using multi-part-upload?

Do you mean the case option.Concurrency <= 1? Both of the ways use s3.CreateMultipartUploadInput.

Do we really need to care about these 2 requests? If so, what if we meet an error and don't call complete?

D3Hunter · 2025-09-11T06:39:41Z

 		m.mu.Unlock()
 	}
+	onReaderClose := func(summary *external.ReaderSummary) {
+		m.summary.GetReqCnt.Add(summary.GetRequestCount)


what about PUT req count?

Updated. Let me add an integration test for the merge-sort step.

Signed-off-by: tangenta <tangenta@126.com>

D3Hunter · 2025-09-11T08:03:10Z

-			return errors.Trace(err)
+			return totalKVBytes, errors.Trace(err)
 		}
+		totalKVBytes += len(key) + len(idxVal)


external writer will calculate the written file size, can we directly use that?

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

Line 529 in 9b964af

TotalSize: w.totalSize,

This size is only updated when the writer is flushed or closed. But we collect them during the write.

Let me change to this way in the next PR?

D3Hunter · 2025-09-11T08:33:22Z

please also add some tests for import into

Signed-off-by: tangenta <tangenta@126.com>

ti-chi-bot · 2025-09-15T03:06:55Z

[LGTM Timeline notifier]

Timeline:

2025-09-11 06:10:26.056385229 +0000 UTC m=+514492.616266726: ☑️ agreed by joechenrh.
2025-09-15 03:06:54.921713061 +0000 UTC m=+849081.481594558: ☑️ agreed by D3Hunter.

bb7133

LGTM

bb7133

LGTM

cfzjywxk

For the client-go update

ti-chi-bot · 2025-09-15T07:01:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bb7133, cfzjywxk, D3Hunter, joechenrh, Leavrth

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [D3Hunter,Leavrth,bb7133,cfzjywxk,joechenrh]
~~br/OWNERS~~ [Leavrth]
~~br/pkg/storage/OWNERS~~ [D3Hunter,Leavrth]
~~lightning/OWNERS~~ [D3Hunter]
~~pkg/ddl/OWNERS~~ [D3Hunter]
~~pkg/disttask/OWNERS~~ [D3Hunter]
~~pkg/executor/importer/OWNERS~~ [D3Hunter]
~~pkg/lightning/OWNERS~~ [D3Hunter]

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

Signed-off-by: tangenta <tangenta@126.com>

wjhuang2016 · 2025-09-15T08:46:31Z

/hold

Signed-off-by: tangenta <tangenta@126.com>

wjhuang2016 · 2025-09-15T09:16:52Z

The test is insufficient.
Consider that another component requests this PR to test, it's ok to merge now. But we need to add more tests to ensure it meets our expectations.

tangenta · 2025-09-15T10:49:39Z

/retest

tiprow · 2025-09-15T10:50:01Z

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

Details

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-sigs/prow repository.

tangenta · 2025-09-15T11:55:16Z

/retest

tiprow · 2025-09-15T11:55:38Z

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

Details

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-sigs/prow repository.

disttask: collect metering data to realtime summary

502d363

Signed-off-by: tangenta <tangenta@126.com>

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 Sep 9, 2025

tangenta changed the title ~~disttask: collect metering data to realtime summary~~ disttask: collect metering data to subtask's real time summary Sep 9, 2025

tangenta commented Sep 9, 2025

View reviewed changes

update bazel and fix build

7ef2bda

Signed-off-by: tangenta <tangenta@126.com>

fix linter

ebcb42d

Signed-off-by: tangenta <tangenta@126.com>

joechenrh reviewed Sep 10, 2025

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 Sep 10, 2025

tangenta added 4 commits September 10, 2025 15:13

move put request counter to the buffered writer

9b964af

Signed-off-by: tangenta <tangenta@126.com>

record get request count for concurrent reader

a680e8a

Signed-off-by: tangenta <tangenta@126.com>

use noop collector

8d3d3a9

Signed-off-by: tangenta <tangenta@126.com>

fix iter_test.go

f882f0e

Signed-off-by: tangenta <tangenta@126.com>

D3Hunter reviewed Sep 10, 2025

View reviewed changes

tangenta added 3 commits September 10, 2025 21:33

s3: add two for put request

c3cf29d

Signed-off-by: tangenta <tangenta@126.com>

refine code

c5517bc

Signed-off-by: tangenta <tangenta@126.com>

fix TestMultiUploadErrorNotOverwritten

14b0990

Signed-off-by: tangenta <tangenta@126.com>

joechenrh approved these changes Sep 11, 2025

View reviewed changes

ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Sep 11, 2025

D3Hunter reviewed Sep 11, 2025

View reviewed changes

address comment

3215dc2

Signed-off-by: tangenta <tangenta@126.com>

D3Hunter reviewed Sep 11, 2025

View reviewed changes

Comment thread tests/realtikvtest/addindextest2/global_sort_test.go Outdated

Comment thread tests/realtikvtest/addindextest2/global_sort_test.go Outdated

Comment thread tests/realtikvtest/addindextest2/global_sort_test.go Outdated

tangenta added 3 commits September 12, 2025 11:25

address part of comments

452e6d6

Signed-off-by: tangenta <tangenta@126.com>

fix import into

f74a9cc

Signed-off-by: tangenta <tangenta@126.com>

update bazel

98f0a86

Signed-off-by: tangenta <tangenta@126.com>

D3Hunter approved these changes Sep 15, 2025

View reviewed changes

ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 15, 2025

Leavrth approved these changes Sep 15, 2025

View reviewed changes

tangenta removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 15, 2025

bb7133 approved these changes Sep 15, 2025

View reviewed changes

ti-chi-bot Bot added the approved label Sep 15, 2025

bb7133 approved these changes Sep 15, 2025

View reviewed changes

cfzjywxk approved these changes Sep 15, 2025

View reviewed changes

fix TestGlobalSortMultiFiles

2edcb6e

Signed-off-by: tangenta <tangenta@126.com>

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 15, 2025

fix TestGlobalSortMultiFiles

6a6d837

Signed-off-by: tangenta <tangenta@126.com>

tangenta removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 15, 2025

fix TestGlobalSortMultiFiles

64b75f4

Signed-off-by: tangenta <tangenta@126.com>

ti-chi-bot Bot merged commit eb25197 into pingcap:master Sep 15, 2025
34 checks passed

D3Hunter mentioned this pull request Sep 22, 2025

DXF service on nextgen kernel #61702

Open

D3Hunter mentioned this pull request Nov 12, 2025

dxf: make api request metering more accurate by intercepting request call #64402

Merged

13 tasks

		tools/bin/mockgen -package mock github.com/pingcap/tidb/pkg/lightning/backend Backend,EngineWriter,TargetInfoGetter > br/pkg/mock/backend.go
		tools/bin/mockgen -package mock github.com/pingcap/tidb/pkg/lightning/common ChunkFlushStatus > br/pkg/mock/common.go

	bufSize := WriteBufferSize
	if option != nil && option.PartSize > 0 {
	bufSize = int(option.PartSize)
	}
	uploaderWriter := newBufferedWriter(uploader, bufSize, NoCompression)
	return uploaderWriter, nil

Uh oh!

Conversation

tangenta commented Sep 9, 2025

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

tiprow Bot commented Sep 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

D3Hunter commented Sep 10, 2025

Uh oh!

tiprow Bot commented Sep 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tangenta commented Sep 10, 2025

Uh oh!

D3Hunter left a comment

Choose a reason for hiding this comment

Uh oh!

D3Hunter commented Sep 11, 2025

Uh oh!

tiprow Bot commented Sep 11, 2025

Uh oh!

D3Hunter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ti-chi-bot Bot commented Sep 15, 2025

[LGTM Timeline notifier]

Uh oh!

bb7133 left a comment

Choose a reason for hiding this comment

Uh oh!

bb7133 left a comment

Choose a reason for hiding this comment

Uh oh!

cfzjywxk left a comment

codecov Bot commented Sep 9, 2025 •

edited

Loading