ddl, stats: switch to new struct for create/truncate table #55811

lance6716 · 2024-09-03T06:54:05Z

What problem does this PR solve?

Issue Number: ref #55723

Problem Summary:

What changed and how does it work?

as title

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test

pass existing tests

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: lance6716 <[email protected]>

tiprow · 2024-09-03T06:54:39Z

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

Signed-off-by: lance6716 <[email protected]>

codecov · 2024-09-03T08:09:41Z

Codecov Report

Attention: Patch coverage is 79.62963% with 22 lines in your changes missing coverage. Please review.

Project coverage is 56.8339%. Comparing base (487ef06) to head (0ebda7e).
Report is 8 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #55811         +/-   ##
=================================================
- Coverage   72.8514%   56.8339%   -16.0175%     
=================================================
  Files          1598       1749        +151     
  Lines        444454     631575     +187121     
=================================================
+ Hits         323791     358949      +35158     
- Misses       100734     247905     +147171     
- Partials      19929      24721       +4792

Flag	Coverage Δ
integration	`39.4332% <77.7777%> (?)`
unit	`72.0337% <78.7037%> (+0.0630%)`	⬆️

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

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

lance6716 · 2024-09-03T08:37:52Z

/retest

tiprow · 2024-09-03T08:38:14Z

@lance6716: 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.

lance6716 · 2024-09-03T09:05:42Z

pkg/statistics/handle/ddl/ddl.go

@@ -58,7 +58,7 @@ func (h *ddlHandlerImpl) HandleDDLEvent(t *util.DDLEvent) error {

 	// ActionFlashbackCluster will not create any new stats info
 	// and it's SchemaID alwayws equals to 0, so skip check it.
-	if t.GetType() != model.ActionFlashbackCluster {
+	if t.GetType() != model.ActionFlashbackCluster && t.SchemaChangeEvent == nil {


we skip generating the SchemaChangeEvent for IsMemOrSysDB

fzzf678 · 2024-09-04T01:31:10Z

/retest

tiprow · 2024-09-04T01:31:32Z

@fzzf678: 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.

Rustin170506

Thanks!

Rustin170506 · 2024-09-04T01:54:08Z

pkg/ddl/create_table.go

@@ -44,6 +45,7 @@ import (
 	"github.com/pingcap/tidb/pkg/table/tables"
 	"github.com/pingcap/tidb/pkg/types"
 	driver "github.com/pingcap/tidb/pkg/types/parser_driver"
+	tidb_util "github.com/pingcap/tidb/pkg/util"


Suggested change

tidb_util "github.com/pingcap/tidb/pkg/util"

tidbutil "github.com/pingcap/tidb/pkg/util"

Rustin170506 · 2024-09-04T01:56:11Z

pkg/ddl/create_table.go

-		tbInfo,
-	)
-	asyncNotifyEvent(jobCtx, createTableEvent)
+	if !tidb_util.IsMemOrSysDB(job.SchemaName) {


Not sure if this is a good idea because maybe we should only handle DDL events. It seems like consumer behavior.

I slightly prefer we filter it at producer. Because the modification of these DBs are done at bootstrap, it's more clear to let consumer nodes also use bootstrap phase to handle it, rather than using the pub-sub framework. And all consumers don't bother to filter it.

Or can we do it within asyncNotifyEvent? That would help us to avoid writing it again and again.

Signed-off-by: lance6716 <[email protected]>

time-and-fate

Others LGTM. Approve in advance.

time-and-fate · 2024-09-04T16:30:26Z

pkg/statistics/handle/ddl/ddl.go

@@ -197,13 +164,46 @@ func (h *ddlHandlerImpl) HandleDDLEvent(t *util.DDLEvent) error {
 		}
 	case model.ActionFlashbackCluster:
 		return h.statsWriter.UpdateStatsVersion()
+	default:
+		logutil.StatsLogger().Info("Handle schema change event", zap.Stringer("event", t.SchemaChangeEvent))


Since we already have a log above, we'll print two logs for each create table event. One of them is "none" and the other is the correct information.
Is that expected?

ti-chi-bot · 2024-09-04T16:32:36Z

[LGTM Timeline notifier]

Timeline:

2024-09-04 02:17:41.217108419 +0000 UTC m=+410785.735161348: ☑️ agreed by fzzf678.
2024-09-04 16:32:35.463668002 +0000 UTC m=+462079.981720927: ☑️ agreed by time-and-fate.

Signed-off-by: lance6716 <[email protected]>

lance6716 · 2024-09-05T02:51:18Z

/unhold

lance6716 · 2024-09-05T03:18:12Z

/retest

tiprow · 2024-09-05T03:18:32Z

@lance6716: 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.

lance6716 · 2024-09-05T03:34:49Z

/retest

tiprow · 2024-09-05T03:35:17Z

@lance6716: 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: lance6716 <[email protected]>

fzzf678 · 2024-09-05T04:54:22Z

/retest

tiprow · 2024-09-05T04:54:45Z

@fzzf678: 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: lance6716 <[email protected]>

lance6716 · 2024-09-05T05:51:34Z

/retest

tiprow · 2024-09-05T05:51:55Z

@lance6716: 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.

lance6716 · 2024-09-05T06:42:07Z

/retest

tiprow · 2024-09-05T06:42:29Z

@lance6716: 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: lance6716 <[email protected]>

lance6716 · 2024-09-05T08:03:35Z

/retest

tiprow · 2024-09-05T08:03:58Z

@lance6716: 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: lance6716 <[email protected]>

* origin/master: (33 commits) build(deps): bump github.com/prometheus/common from 0.55.0 to 0.57.0 (pingcap#55762) build(deps): bump golang.org/x/sys from 0.24.0 to 0.25.0 (pingcap#55894) planner: fix incorrect estRows with global index and json column (pingcap#55842) ddl, stat: switch to new struct for add/truncate/drop partition (pingcap#55893) planner: hide instance plan cache eviction log if no plan is evicted (pingcap#55918) expression: support tidb encode key function (pingcap#51678) planner: fix incorrect maintenance of `handleColHelper` for recursive CTE (pingcap#55732) executor: some code refine of hash join v2 (pingcap#55887) infoschema, meta: fix wrong auto id after `rename table` (pingcap#55847) ddl/ingest: set `minCommitTS` when detect remote duplicate keys (pingcap#55588) planner: move index advisor into the kernel (pingcap#55874) ddl, stats: switch to new struct for create/truncate table (pingcap#55811) executor: avoid new small objects in probe stage of hash join v2 (pingcap#55855) *: Add tidbCPU/tikvCPU into system tables (pingcap#55455) ddl: use static contexts in `NewReorgCopContext` (pingcap#55823) executor: fix index out of range bug in hash join v2 (pingcap#55864) executor: record index usage for the clustered primary keys (pingcap#55602) domain: load all non-public tables into infoschema (pingcap#55142) test: fix unstable TestShowViewPriv (pingcap#55868) ttl: add `varbinary` case for `TestSplitTTLScanRangesWithBytes` (pingcap#55863) ...

ddl, stats: switch to new struct for create/truncate table

cb2a425

Signed-off-by: lance6716 <[email protected]>

ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 3, 2024

ti-chi-bot bot removed do-not-merge/needs-linked-issue do-not-merge/needs-triage-completed labels Sep 3, 2024

lance6716 added 6 commits September 3, 2024 15:04

fix CI

b9178cd

Signed-off-by: lance6716 <[email protected]>

fix lint

d5e92d9

Signed-off-by: lance6716 <[email protected]>

make fmt

282610c

Signed-off-by: lance6716 <[email protected]>

revert some change

b74435d

Signed-off-by: lance6716 <[email protected]>

fix lint

faf1dd9

Signed-off-by: lance6716 <[email protected]>

fix CI

a5eee63

Signed-off-by: lance6716 <[email protected]>

lance6716 commented Sep 3, 2024

View reviewed changes

Rustin170506 reviewed Sep 4, 2024

View reviewed changes

Rustin170506 requested a review from time-and-fate September 4, 2024 01:56

address comment

a9d89b2

Signed-off-by: lance6716 <[email protected]>

fzzf678 approved these changes Sep 4, 2024

View reviewed changes

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

time-and-fate approved these changes Sep 4, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 4, 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 5, 2024

address comment

a0a25a4

Signed-off-by: lance6716 <[email protected]>

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 5, 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 Sep 5, 2024

lance6716 added 2 commits September 5, 2024 11:55

fix UT stuck

2ccb822

Signed-off-by: lance6716 <[email protected]>

Merge branch 'master' of github.com:pingcap/tidb into fix55723-0903

aee34b8

lance6716 added 2 commits September 5, 2024 13:06

remove a UT

1dfd0dc

Signed-off-by: lance6716 <[email protected]>

fix bazel

d256a28

Signed-off-by: lance6716 <[email protected]>

lance6716 added 3 commits September 5, 2024 15:25

remove more UT

8481c15

Signed-off-by: lance6716 <[email protected]>

fix bazel

c1f4fa1

Signed-off-by: lance6716 <[email protected]>

change test content

8adc9e6

Signed-off-by: lance6716 <[email protected]>

refactor tests

0ebda7e

Signed-off-by: lance6716 <[email protected]>

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 5, 2024

ti-chi-bot bot merged commit c90168e into pingcap:master Sep 5, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl, stats: switch to new struct for create/truncate table #55811

ddl, stats: switch to new struct for create/truncate table #55811

lance6716 commented Sep 3, 2024 •

edited

Loading

tiprow bot commented Sep 3, 2024

codecov bot commented Sep 3, 2024 •

edited

Loading

lance6716 commented Sep 3, 2024

tiprow bot commented Sep 3, 2024

lance6716 Sep 3, 2024

fzzf678 commented Sep 4, 2024

tiprow bot commented Sep 4, 2024

Rustin170506 left a comment

Rustin170506 Sep 4, 2024

Rustin170506 Sep 4, 2024

lance6716 Sep 4, 2024 •

edited

Loading

Rustin170506 Sep 5, 2024

lance6716 Sep 5, 2024

time-and-fate left a comment

time-and-fate Sep 4, 2024

lance6716 Sep 5, 2024

ti-chi-bot bot commented Sep 4, 2024

lance6716 commented Sep 5, 2024

lance6716 commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

lance6716 commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

fzzf678 commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

lance6716 commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

lance6716 commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

lance6716 commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

	tidb_util "github.com/pingcap/tidb/pkg/util"
	tidbutil "github.com/pingcap/tidb/pkg/util"

ddl, stats: switch to new struct for create/truncate table #55811

ddl, stats: switch to new struct for create/truncate table #55811

Conversation

lance6716 commented Sep 3, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Sep 3, 2024

codecov bot commented Sep 3, 2024 • edited Loading

Codecov Report

lance6716 commented Sep 3, 2024

tiprow bot commented Sep 3, 2024

lance6716 Sep 3, 2024

Choose a reason for hiding this comment

fzzf678 commented Sep 4, 2024

tiprow bot commented Sep 4, 2024

Rustin170506 left a comment

Choose a reason for hiding this comment

Rustin170506 Sep 4, 2024

Choose a reason for hiding this comment

Rustin170506 Sep 4, 2024

Choose a reason for hiding this comment

lance6716 Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Rustin170506 Sep 5, 2024

Choose a reason for hiding this comment

lance6716 Sep 5, 2024

Choose a reason for hiding this comment

time-and-fate left a comment

Choose a reason for hiding this comment

time-and-fate Sep 4, 2024

Choose a reason for hiding this comment

lance6716 Sep 5, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 4, 2024

[LGTM Timeline notifier]

lance6716 commented Sep 5, 2024

lance6716 commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

lance6716 commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

fzzf678 commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

lance6716 commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

lance6716 commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

lance6716 commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

lance6716 commented Sep 3, 2024 •

edited

Loading

codecov bot commented Sep 3, 2024 •

edited

Loading

lance6716 Sep 4, 2024 •

edited

Loading