ddl, stat: switch to new struct for remove partition, flash back and remove `DDLEvent` #55973

fzzf678 · 2024-09-10T06:41:44Z

What problem does this PR solve?

Issue Number: ref #55723

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-09-10T06:42:02Z

Hi @fzzf678. 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-09-10T06:53:08Z

Codecov Report

Attention: Patch coverage is 87.50000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 57.2276%. Comparing base (ea6dba8) to head (8b171a4).
Report is 11 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #55973         +/-   ##
=================================================
- Coverage   72.8567%   57.2276%   -15.6292%     
=================================================
  Files          1605       1755        +150     
  Lines        446200     634824     +188624     
=================================================
+ Hits         325087     363295      +38208     
- Misses       101079     246800     +145721     
- Partials      20034      24729       +4695

Flag	Coverage Δ
integration	`39.7298% <72.1153%> (?)`
unit	`72.2126% <76.9230%> (+0.2341%)`	⬆️

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

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

lance6716

rest lgtm

pkg/ddl/util/schema_change_notifier.go

pkg/statistics/handle/ddl/ddl.go

Rustin170506

Thansk!

Rustin170506 · 2024-09-10T07:58:09Z

pkg/ddl/util/util_test.go

+	result := e.String()
+
+	// Check the result
+	expected := "(Event Type: add column, Table ID: 1, Table Name: Table1, Old Table ID: 4, Old Table Name: Table2, Partition Name: , Partition ID: 2, Partition Name: , Partition ID: 3, Dropped Partition Name: , Dropped Partition ID: 5, Dropped Partition Name: , Dropped Partition ID: 6, Column ID: 7, Column Name: Column1, Column ID: 8, Column Name: Column2)"


Do we need to print an empty string if the name is empty?

fixed, only print non-empty name, PTAL again

pkg/statistics/handle/ddl/ddl.go

Rustin170506 · 2024-09-10T08:00:35Z

pkg/statistics/handle/ddl/ddl.go

-		if isSysDB, err := t.IsMemOrSysDB(sctx.(sessionctx.Context)); err != nil {
-			return err
-		} else if isSysDB {
-			// EXCHANGE PARTITION EVENT NOTES:


Better to keep this comment somewhere. We allow users to exchange one normal table for a system table.

Rustin170506

rest LGTM

/hold

Feel free to unhold after you address them.

pkg/ddl/util/schema_change_notifier.go

pkg/statistics/handle/ddl/ddl.go

ti-chi-bot · 2024-09-10T09:09:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance6716, Rustin170506

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:

~~pkg/ddl/OWNERS~~ [lance6716]
~~pkg/statistics/OWNERS~~ [Rustin170506]

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-09-10T09:09:20Z

[LGTM Timeline notifier]

Timeline:

2024-09-10 06:58:03.163088673 +0000 UTC m=+339552.903512605: ☑️ agreed by lance6716.
2024-09-10 09:09:19.749232423 +0000 UTC m=+347429.489656365: ☑️ agreed by Rustin170506.

Co-authored-by: Rustin <[email protected]>

pkg/statistics/handle/ddl/ddl.go

Rustin170506 · 2024-09-10T10:05:48Z

pkg/statistics/handle/ddl/ddl.go

+	case model.ActionFlashbackCluster:
+		return h.statsWriter.UpdateStatsVersion()
+	default:
+		return errors.Trace(errors.Errorf("unhandled DDL type %v", s.GetType()))


I think just an error log would be enough.

maybe use intest to remind PR author when adding new event types.

fzzf678 · 2024-09-10T10:10:30Z

/unhold

Rustin170506 · 2024-09-10T10:14:12Z

/hold

#55973 (comment)

Rustin170506 · 2024-09-10T11:14:29Z

pkg/statistics/handle/ddl/ddl.go

@@ -183,7 +186,8 @@ func (h *ddlHandlerImpl) HandleDDLEvent(s *ddlutil.SchemaChangeEvent) error {
 	case model.ActionFlashbackCluster:
 		return h.statsWriter.UpdateStatsVersion()
 	default:
-		return errors.Trace(errors.Errorf("unhandled DDL type %v", s.GetType()))
+		intest.Assert(false)
+		logutil.StatsLogger().Error("Unhandled schema change event", zap.Stringer("type", s.GetType()))


Do we need to print the whole event here?

What’s the purpose of printing the whole event in log?

I just thought that the purpose of this default branch is to remind developer there is an unhandled event, any other use? 🤔

I guess it would be easier to figure out what information is in it. But I am okay with it if you only want to print the type here.

PTAL again 8b171a4

Rustin170506 · 2024-09-11T03:50:22Z

/unhold

Thank you very much.

…remove `DDLEvent` (pingcap#55973) ref pingcap#55723

fzzf678 added 7 commits September 10, 2024 13:05

replace

89150c7

rename field

de7ef46

remove useless func

e5f3826

replace DDLEvent

22cfd23

remove DDLEvent

d7019e3

sort import

5cf10ff

update bazel

f4a2bba

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 10, 2024

lance6716 approved these changes Sep 10, 2024

View reviewed changes

pkg/ddl/util/schema_change_notifier.go Outdated Show resolved Hide resolved

pkg/ddl/util/schema_change_notifier.go Outdated Show resolved Hide resolved

pkg/statistics/handle/ddl/ddl.go Outdated Show resolved Hide resolved

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

fzzf678 added 3 commits September 10, 2024 15:06

fix rename and line style

4813ed0

remove unused param

b117651

remove todo

59a52a1

fzzf678 requested review from Rustin170506 and hawkingrei September 10, 2024 07:52

Rustin170506 reviewed Sep 10, 2024

View reviewed changes

fzzf678 added 4 commits September 10, 2024 16:15

print partition name when it's not empty

f9175e4

supply comment

0f76ff3

add default branch

03a6621

||

ece1cd2

Rustin170506 approved these changes Sep 10, 2024

View reviewed changes

pkg/ddl/util/schema_change_notifier.go Outdated Show resolved Hide resolved

pkg/statistics/handle/ddl/ddl.go Show resolved Hide resolved

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

ti-chi-bot bot added approved lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 10, 2024

fzzf678 and others added 2 commits September 10, 2024 17:14

Update pkg/statistics/handle/ddl/ddl.go

67265d9

Co-authored-by: Rustin <[email protected]>

Update pkg/ddl/util/schema_change_notifier.go

7fd4d01

Co-authored-by: Rustin <[email protected]>

Rustin170506 reviewed Sep 10, 2024

View reviewed changes

pkg/statistics/handle/ddl/ddl.go Outdated Show resolved Hide resolved

update comment

627ff17

Rustin170506 reviewed Sep 10, 2024

View reviewed changes

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

log error in default branch

a04f0ca

Rustin170506 reviewed Sep 10, 2024

View reviewed changes

Update ddl.go

8b171a4

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 Sep 11, 2024

ti-chi-bot bot merged commit 2ea3542 into pingcap:master Sep 11, 2024
23 checks passed

Benjamin2037 pushed a commit to Benjamin2037/tidb that referenced this pull request Sep 11, 2024

ddl, stat: switch to new struct for remove partition, flash back and …

d178713

…remove `DDLEvent` (pingcap#55973) ref pingcap#55723

Benjamin2037 pushed a commit to Benjamin2037/tidb that referenced this pull request Sep 11, 2024

ddl, stat: switch to new struct for remove partition, flash back and …

b662e00

…remove `DDLEvent` (pingcap#55973) ref pingcap#55723

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl, stat: switch to new struct for remove partition, flash back and remove `DDLEvent` #55973

ddl, stat: switch to new struct for remove partition, flash back and remove `DDLEvent` #55973

fzzf678 commented Sep 10, 2024

tiprow bot commented Sep 10, 2024

codecov bot commented Sep 10, 2024 •

edited

Loading

lance6716 left a comment

Rustin170506 left a comment

Rustin170506 Sep 10, 2024

fzzf678 Sep 10, 2024

Rustin170506 Sep 10, 2024

Rustin170506 left a comment

ti-chi-bot bot commented Sep 10, 2024

ti-chi-bot bot commented Sep 10, 2024

Rustin170506 Sep 10, 2024

lance6716 Sep 10, 2024

fzzf678 commented Sep 10, 2024

Rustin170506 commented Sep 10, 2024

Rustin170506 Sep 10, 2024

fzzf678 Sep 10, 2024

fzzf678 Sep 10, 2024

Rustin170506 Sep 10, 2024

fzzf678 Sep 11, 2024

Rustin170506 commented Sep 11, 2024

ddl, stat: switch to new struct for remove partition, flash back and remove DDLEvent #55973

ddl, stat: switch to new struct for remove partition, flash back and remove DDLEvent #55973

Conversation

fzzf678 commented Sep 10, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Sep 10, 2024

codecov bot commented Sep 10, 2024 • edited Loading

Codecov Report

lance6716 left a comment

Choose a reason for hiding this comment

Rustin170506 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rustin170506 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 10, 2024

ti-chi-bot bot commented Sep 10, 2024

[LGTM Timeline notifier]

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fzzf678 commented Sep 10, 2024

Rustin170506 commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rustin170506 commented Sep 11, 2024

ddl, stat: switch to new struct for remove partition, flash back and remove `DDLEvent` #55973

ddl, stat: switch to new struct for remove partition, flash back and remove `DDLEvent` #55973

codecov bot commented Sep 10, 2024 •

edited

Loading