dm: add TCP connection IO traffic statistics to sync stage status in OpenAPI response #11742

River2000i · 2024-11-13T04:29:12Z

What problem does this PR solve?

Issue Number: close #11741 #11746

What is changed and how it works?

add filed in dm/proto/dmworker.proto and regenerate by make generate-protobuf
update openapi define and regenerate by make dm_generate_openapi
init IO counter and uuid for the task created by openapi

Check List

Tests

Unit test
Manual test (add detailed scripts or steps below)

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

Please refer to [Release Notes Language Style Guide](https://pingcap.github.io/tidb-dev-guide/contribute-to-tidb/release-notes-style-guide.html) to write a quality release note.

If you don't think this PR needs a release note then fill it with `None`.

ti-chi-bot · 2024-11-13T04:29:22Z

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

lance6716 · 2024-11-14T03:12:27Z

dm/config/subtask.go

@@ -182,13 +182,13 @@ type SubTaskConfig struct {
 	// one go runtime.
 	// IOTotalBytes is used build TCPConnWithIOCounter and UUID is used to as a
 	// key to let MySQL driver to find the right TCPConnWithIOCounter.
-	UUID         string         `toml:"-" json:"-"`
-	IOTotalBytes *atomic.Uint64 `toml:"-" json:"-"`
+	UUID         string         `toml:"uuid" json:"uuid"`


I remember toml is used for passing task content, and json is used for logging. We don't need to log them?

Also please add unit tests to ensure atomic types has implemented these marshall functions without race

I remember toml is used for passing task content, and json is used for logging. We don't need to log them?

Indeed, we don't need to add json tag for both uuid. json tag used for log and openapi response

Also please add unit tests to ensure atomic types has implemented these marshall functions without race

nice catch! The atomic.Uint64 values can't be properly serialized to TOML format. How about we manually copy atomic value like:b094cb4

since atomic.Uint64 is not support marshall functions for toml like https://github.com/pingcap/tiflow/blob/master/dm/master/scheduler/worker.go#L282

we need to avoid (nil pointer)/(zero value) after covert subtaskconfig to toml... @lance6716 Any suggestion for that?🤔

lance6716 · 2024-11-14T07:08:26Z

/ok-to-test

River2000i · 2024-11-14T08:18:42Z

/retest

River2000i · 2024-11-14T08:23:53Z

/retest-required

River2000i · 2024-11-25T02:47:47Z

@GMHDBJD @D3Hunter PTAL~

River2000i · 2024-11-26T03:41:53Z

dm/tests/openapi/run.sh


 	init_dump_data

 	# start dump task success
 	openapi_task_check "start_task_success" $task_name ""
-	run_dm_ctl_with_retry $WORK_DIR "127.0.0.1:$MASTER_PORT" \
-		"query-status $task_name" \
-		"\"stage\": \"Running\"" 1


in some case, dump task will finished immediately so cannot get the Running stage. Only validate task finished or not in test_dump_task(). Need to validate dump data after we support load task.

River2000i · 2024-11-26T06:49:56Z

/retest

D3Hunter

rest lgtm

D3Hunter · 2024-11-27T02:06:12Z

dm/tests/openapi/client/openapi_task_check

+    max_dump_io_total_bytes=sys.maxsize,
+    min_io_total_bytes=0,
+    max_io_total_bytes=sys.maxsize


The value sys.maxsize, on the other hand, reports the platform's pointer size, and that limits the size of Python's data structures such as strings and lists.

maybe use a more restricted range, such as a few KiB or MiB

Set max to 100 KiB. In openapi test, all sync task is less than 30KiB.

what about the minimum? i guess we will at least send some rows for DM to sync. we want to check that the bytes are in a reasonable range to meter correct

For sure, for now the minimum is ~3KiB. I think we can set to 2KiB for stable test. And maximum set to 50KiB.

D3Hunter

rest lgtm

D3Hunter · 2024-11-27T06:23:45Z

/hold

please fix existing comments, you can unhold it youself

River2000i · 2024-11-27T08:59:00Z

/retest

River2000i · 2024-11-27T09:25:47Z

/retest

River2000i · 2024-11-27T12:39:06Z

/retest

GMHDBJD

LGTM

ti-chi-bot · 2024-11-28T02:40:37Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,GMHDBJD]
~~dm/OWNERS~~ [D3Hunter,GMHDBJD]
~~engine/OWNERS~~ [D3Hunter,GMHDBJD]

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-11-28T02:40:39Z

[LGTM Timeline notifier]

Timeline:

2024-11-27 06:23:24.984399899 +0000 UTC m=+617592.604054414: ☑️ agreed by D3Hunter.
2024-11-28 02:40:38.504127318 +0000 UTC m=+690626.123781842: ☑️ agreed by GMHDBJD.

River2000i · 2024-11-28T03:31:02Z

/unhold

River2000i added 7 commits November 13, 2024 12:07

add toml\json field

069b97c

init subtask config from openapi

c78282e

retrun ioconter in status

4d93319

regenerate openapi define

955ee74

update grpc proto

1758099

update to github.com/golang/[email protected] and generate-protobuf

5adf6df

add test

842c5eb

ti-chi-bot bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 13, 2024

River2000i changed the title ~~dm: add IO traffic statistics to sync stage status in OpenAPI response~~ dm: add TCP connection IO traffic statistics to sync stage status in OpenAPI response Nov 13, 2024

lance6716 reviewed Nov 14, 2024

View reviewed changes

River2000i added 3 commits November 14, 2024 13:20

remove json tag for uuid

00b827a

manual clone atomic.Uint64 and add test

b094cb4

fix test

e3b5bcb

ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Nov 14, 2024

River2000i added 3 commits November 14, 2024 15:50

fix nil pointer...

38ff844

Merge branch 'master' into featIoTotalBytes

02c3793

Merge branch 'master' into featIoTotalBytes

94ae7cf

River2000i added 2 commits November 14, 2024 16:28

fix build

5c2c5ae

fix nil

91304ea

River2000i added 3 commits November 23, 2024 14:37

dump task only check finished

78c1502

only check task finished

43ea410

fix

fc513b2

River2000i commented Nov 26, 2024

View reviewed changes

D3Hunter reviewed Nov 27, 2024

View reviewed changes

River2000i added 3 commits November 27, 2024 12:03

restricted max io total bytes in check_sync_task_status_success()

68fffb3

fix

633458b

fix

7ec18bd

River2000i requested a review from D3Hunter November 27, 2024 05:38

D3Hunter approved these changes Nov 27, 2024

View reviewed changes

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

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 Nov 27, 2024

River2000i added 2 commits November 27, 2024 15:46

update sync task checker for io min/max

e0b40a1

Merge branch 'master' into featIoTotalBytes

b6223d0

GMHDBJD approved these changes Nov 28, 2024

View reviewed changes

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

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 Nov 28, 2024

ti-chi-bot bot merged commit b909eff into pingcap:master Nov 28, 2024
26 checks passed

River2000i deleted the featIoTotalBytes branch November 28, 2024 03:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dm: add TCP connection IO traffic statistics to sync stage status in OpenAPI response #11742

dm: add TCP connection IO traffic statistics to sync stage status in OpenAPI response #11742

River2000i commented Nov 13, 2024 •

edited

Loading

ti-chi-bot bot commented Nov 13, 2024

lance6716 Nov 14, 2024

River2000i Nov 14, 2024

River2000i Nov 14, 2024

River2000i Nov 14, 2024

lance6716 commented Nov 14, 2024

River2000i commented Nov 14, 2024

River2000i commented Nov 14, 2024

River2000i commented Nov 25, 2024

River2000i Nov 26, 2024 •

edited

Loading

River2000i commented Nov 26, 2024

D3Hunter left a comment

D3Hunter Nov 27, 2024

River2000i Nov 27, 2024

D3Hunter Nov 27, 2024

River2000i Nov 27, 2024 •

edited

Loading

D3Hunter left a comment

D3Hunter commented Nov 27, 2024

River2000i commented Nov 27, 2024

River2000i commented Nov 27, 2024

River2000i commented Nov 27, 2024

GMHDBJD left a comment

ti-chi-bot bot commented Nov 28, 2024

ti-chi-bot bot commented Nov 28, 2024

River2000i commented Nov 28, 2024

dm: add TCP connection IO traffic statistics to sync stage status in OpenAPI response #11742

dm: add TCP connection IO traffic statistics to sync stage status in OpenAPI response #11742

Conversation

River2000i commented Nov 13, 2024 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Tests

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

ti-chi-bot bot commented Nov 13, 2024

lance6716 Nov 14, 2024

Choose a reason for hiding this comment

River2000i Nov 14, 2024

Choose a reason for hiding this comment

River2000i Nov 14, 2024

Choose a reason for hiding this comment

River2000i Nov 14, 2024

Choose a reason for hiding this comment

lance6716 commented Nov 14, 2024

River2000i commented Nov 14, 2024

River2000i commented Nov 14, 2024

River2000i commented Nov 25, 2024

River2000i Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

River2000i commented Nov 26, 2024

D3Hunter left a comment

Choose a reason for hiding this comment

D3Hunter Nov 27, 2024

Choose a reason for hiding this comment

River2000i Nov 27, 2024

Choose a reason for hiding this comment

D3Hunter Nov 27, 2024

Choose a reason for hiding this comment

River2000i Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

D3Hunter left a comment

Choose a reason for hiding this comment

D3Hunter commented Nov 27, 2024

River2000i commented Nov 27, 2024

River2000i commented Nov 27, 2024

River2000i commented Nov 27, 2024

GMHDBJD left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 28, 2024

ti-chi-bot bot commented Nov 28, 2024

[LGTM Timeline notifier]

River2000i commented Nov 28, 2024

River2000i commented Nov 13, 2024 •

edited

Loading

River2000i Nov 26, 2024 •

edited

Loading

River2000i Nov 27, 2024 •

edited

Loading