ttl: fix the issue that TTL job may hang some time when shrink the delete worker count #55572

lcwangchao · 2024-08-21T12:40:00Z

What problem does this PR solve?

Issue Number: close #55561

What changed and how does it work?

fix some missed count for ErrorRows
if a scan task is finished, it will wait for all rows to be processed. If it is not finished in a certain time, the task will go one to mark itself as finished.

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-08-21T12:40:22Z

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

codecov · 2024-08-21T13:11:30Z

Codecov Report

Attention: Patch coverage is 95.38462% with 3 lines in your changes missing coverage. Please review.

Project coverage is 75.0851%. Comparing base (1f095a3) to head (8cbad54).
Report is 25 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #55572        +/-   ##
================================================
+ Coverage   73.0193%   75.0851%   +2.0658%     
================================================
  Files          1576       1582         +6     
  Lines        440864     453011     +12147     
================================================
+ Hits         321916     340144     +18228     
+ Misses        99271      92389      -6882     
- Partials      19677      20478       +801

Flag	Coverage Δ
integration	`48.9622% <6.1538%> (?)`
unit	`71.8784% <95.3846%> (-0.2205%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.9567% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`52.5244% <ø> (+6.5651%)`	⬆️

…lete worker count

bb7133

LGTM

YangKeao · 2024-08-23T05:02:44Z

pkg/ttl/ttlworker/task_manager.go

+		// All rows are processed.
+		// We use `<=` instead of `==` to make the logic strong to make sure
+		// it also works when statistics are not accurate.
+		logger.Info("mark TTL task finished because all scanned rows are processed")


How about adding a warning for the case t.statistics.ErrorRows.Load()+t.statistics.SuccessRows.Load() > t.statistics.TotalRows.Load(), as we currently don't know when they are not equal.

YangKeao · 2024-08-23T05:03:41Z

pkg/ttl/ttlworker/task_manager.go

+		return true
+	}
+
+	if time.Since(t.result.time) > waitTaskProcessRowsTimeout {


Is two minutes long enough? Previously, the meaning of finished() is that both scan and delete are finished. If we are going to keep the behavior, it means the delete should have finished within 2 minutes, right?

Maybe I can enlarge it to 5 minutes. The delChan is a channel with 0 zero size, and san worker will only be marked as finished after the delTask emitted. That means we as most waiting tidb_ttl_scan_batch_size rows to deleted. tidb_ttl_scan_batch_size default value is 500, I think it is enough for normal cases

YangKeao

LGTM

ti-chi-bot · 2024-08-23T06:09:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bb7133, YangKeao

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~~ [YangKeao,bb7133]

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-08-23T06:10:01Z

[LGTM Timeline notifier]

Timeline:

2024-08-22 17:49:35.887402351 +0000 UTC m=+460571.021852472: ☑️ agreed by bb7133.
2024-08-23 06:10:00.614894826 +0000 UTC m=+504995.749344952: ☑️ agreed by YangKeao.

lcwangchao · 2024-08-23T06:37:57Z

/retest

tiprow · 2024-08-23T06:38:19Z

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

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2024-08-23T07:11:28Z

In response to a cherrypick label: new pull request created to branch release-7.1: #55612.

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2024-08-23T07:12:10Z

In response to a cherrypick label: new pull request created to branch release-7.5: #55613.

ti-chi-bot · 2024-08-23T07:12:50Z

In response to a cherrypick label: new pull request created to branch release-8.1: #55614.

…lete worker count (#55572) (#55613) close #55561

…lete worker count (#55572) (#55612) close #55561

…lete worker count (#55572) (#55614) close #55561

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 21, 2024

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Aug 21, 2024

lcwangchao added needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. labels Aug 21, 2024

lcwangchao force-pushed the fix_ttl_hang branch 3 times, most recently from b47a900 to 3944d30 Compare August 21, 2024 12:58

lcwangchao force-pushed the fix_ttl_hang branch from 3944d30 to a60e2f5 Compare August 21, 2024 13:23

ttl: fix the issue that TTL job may hang some time when shrink the de…

49242d0

…lete worker count

lcwangchao force-pushed the fix_ttl_hang branch from a60e2f5 to 49242d0 Compare August 21, 2024 13:26

lcwangchao requested review from YangKeao and bb7133 August 22, 2024 02:13

bb7133 approved these changes Aug 22, 2024

View reviewed changes

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

YangKeao reviewed Aug 23, 2024

View reviewed changes

YangKeao approved these changes Aug 23, 2024

View reviewed changes

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

lcwangchao force-pushed the fix_ttl_hang branch from 3b299ac to 4b35fc9 Compare August 23, 2024 06:11

address comment

8cbad54

lcwangchao force-pushed the fix_ttl_hang branch from 4b35fc9 to 8cbad54 Compare August 23, 2024 06:13

ti-chi-bot bot merged commit 1bf01f4 into pingcap:master Aug 23, 2024
21 checks passed

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Aug 23, 2024

This is an automated cherry-pick of pingcap#55572

66be4e7

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Aug 23, 2024

ttl: fix the issue that TTL job may hang some time when shrink the delete worker count (#55572) #55612

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Aug 23, 2024

This is an automated cherry-pick of pingcap#55572

3564f91

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Aug 23, 2024

ttl: fix the issue that TTL job may hang some time when shrink the delete worker count (#55572) #55613

Merged

13 tasks

ti-chi-bot mentioned this pull request Aug 23, 2024

ttl: fix the issue that TTL job may hang some time when shrink the delete worker count (#55572) #55614

Merged

13 tasks

lcwangchao deleted the fix_ttl_hang branch August 23, 2024 08:03

ti-chi-bot bot pushed a commit that referenced this pull request Aug 23, 2024

ttl: fix the issue that TTL job may hang some time when shrink the de…

ad786db

…lete worker count (#55572) (#55613) close #55561

ti-chi-bot bot pushed a commit that referenced this pull request Aug 23, 2024

ttl: fix the issue that TTL job may hang some time when shrink the de…

4ac668d

…lete worker count (#55572) (#55612) close #55561

ti-chi-bot bot pushed a commit that referenced this pull request Aug 27, 2024

ttl: fix the issue that TTL job may hang some time when shrink the de…

b22d1ea

…lete worker count (#55572) (#55614) close #55561

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ttl: fix the issue that TTL job may hang some time when shrink the delete worker count #55572

ttl: fix the issue that TTL job may hang some time when shrink the delete worker count #55572

Uh oh!

lcwangchao commented Aug 21, 2024

Uh oh!

tiprow bot commented Aug 21, 2024

Uh oh!

codecov bot commented Aug 21, 2024 •

edited

Loading

Uh oh!

bb7133 left a comment

Uh oh!

YangKeao Aug 23, 2024

Uh oh!

YangKeao Aug 23, 2024

Uh oh!

lcwangchao Aug 23, 2024

Uh oh!

YangKeao left a comment

Uh oh!

ti-chi-bot bot commented Aug 23, 2024

Uh oh!

ti-chi-bot bot commented Aug 23, 2024

Uh oh!

lcwangchao commented Aug 23, 2024

Uh oh!

tiprow bot commented Aug 23, 2024

Uh oh!

Uh oh!

ti-chi-bot commented Aug 23, 2024

Uh oh!

ti-chi-bot commented Aug 23, 2024

Uh oh!

ti-chi-bot commented Aug 23, 2024

Uh oh!

Uh oh!

ttl: fix the issue that TTL job may hang some time when shrink the delete worker count #55572

ttl: fix the issue that TTL job may hang some time when shrink the delete worker count #55572

Uh oh!

Conversation

lcwangchao commented Aug 21, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

tiprow bot commented Aug 21, 2024

Uh oh!

codecov bot commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bb7133 left a comment

Choose a reason for hiding this comment

Uh oh!

YangKeao Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

YangKeao Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

lcwangchao Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

YangKeao left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Aug 23, 2024

Uh oh!

ti-chi-bot bot commented Aug 23, 2024

[LGTM Timeline notifier]

Uh oh!

lcwangchao commented Aug 23, 2024

Uh oh!

tiprow bot commented Aug 23, 2024

Uh oh!

Uh oh!

ti-chi-bot commented Aug 23, 2024

Uh oh!

ti-chi-bot commented Aug 23, 2024

Uh oh!

ti-chi-bot commented Aug 23, 2024

Uh oh!

Uh oh!

codecov bot commented Aug 21, 2024 •

edited

Loading