Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ddl: add the ddl notifier as a listener to the stats owner #56795

Merged
merged 5 commits into from
Oct 25, 2024
Merged

ddl: add the ddl notifier as a listener to the stats owner #56795

merged 5 commits into from
Oct 25, 2024

Conversation

Rustin170506
Copy link
Member

What problem does this PR solve?

Issue Number: ref #55722

Problem Summary:

What changed and how does it work?

We should only start the ddl notifier on the stats owner node. So in this PR, I added the ddl notifier as a listener to the stats owner.

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

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 23, 2024
@codecov Codecov
Copy link

codecov bot commented Oct 23, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 76.74419% with 10 lines in your changes missing coverage. Please review.

Project coverage is 57.0222%. Comparing base (ed9a909) to head (42b37d9).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@                Coverage Diff                @@
##             master     #56795         +/-   ##
=================================================
- Coverage   73.2825%   57.0222%   -16.2604%     
=================================================
  Files          1636       1786        +150     
  Lines        453340     640198     +186858     
=================================================
+ Hits         332219     365055      +32836     
- Misses       100768     250025     +149257     
- Partials      20353      25118       +4765
Flag Coverage Δ
integration 39.3174% <62.7907%> (?)
unit 72.4945% <76.7441%> (+0.0089%) ⬆️

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

Components Coverage Δ
dumpling 52.9478% <ø> (ø)
parser ∅ <ø> (∅)
br 63.0776% <ø> (+17.0863%) ⬆️

@Rustin170506 Rustin170506 requested review from lance6716 and fzzf678 and removed request for lance6716 October 23, 2024 07:04
@Rustin170506 Rustin170506 added the component/ddl This issue is related to DDL of TiDB. label Oct 23, 2024
Rustin170506
Rustin170506 commented Oct 23, 2024
View reviewed changes
Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔢 Self-check (PR reviewed by myself and ready for feedback.)

Rustin170506
Rustin170506 commented Oct 23, 2024
View reviewed changes
Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔢 Self-check (PR reviewed by myself and ready for feedback.)

lance6716
lance6716 reviewed Oct 23, 2024
View reviewed changes
pkg/ddl/notifier/subscribe.go
// 2. Keeping the stats handler and DDLNotifier on the same node maintains data integrity.
// 3. It prevents potential race conditions or inconsistencies that could arise from
// distributed processing of these events across multiple nodes.
var _ owner.Listener = (*DDLNotifier)(nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to remind that owner.Listener can not ensure very strict "only one owner in database". If the old owner retires slowly the new owner will cut in.

@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 23, 2024
@Rustin170506
Copy link
Member Author

/retest

Rustin170506
Rustin170506 commented Oct 24, 2024
View reviewed changes
Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔢 Self-check (PR reviewed by myself and ready for feedback.)

@Rustin170506
Copy link
Member Author

/retest

@Rustin170506 Rustin170506 force-pushed the rustin-patch-ddl-stats-owner branch from 7637e53 to 39a1371 Compare October 24, 2024 07:27
@Rustin170506 Rustin170506 mentioned this pull request Oct 24, 2024
Merged
13 tasks
@Rustin170506
ddl: add the ddl notifier as a listener to stats owner
97071da 
Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>

ddl: close the ddl notifier correctly

Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>

ddl: better comment

Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>

ddl: fix broken tests

Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>

ddl: use wait group to wait the goroutine exits

Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>

ddl: update bazel files

Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>

refactor: remove useless wait

Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>

fix: move the wait to the right position

Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>

fix: rename and update comment

Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>

fix: update comment

Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>

fix: use system session pool

Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>

docs: update comments

Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>

fix: make bazel happy

Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>

refactor: simplify test code

Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>

fix: make bazel happy

Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>
@Rustin170506 Rustin170506 force-pushed the rustin-patch-ddl-stats-owner branch from 39a1371 to 97071da Compare October 24, 2024 07:51
lance6716
lance6716 approved these changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel free to unhold

@lance6716
Copy link
Contributor

/hold

@ti-chi-bot ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 24, 2024
@ti-chi-bot ti-chi-bot
Copy link

ti-chi-bot bot commented Oct 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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 ti-chi-bot bot added the lgtm label Oct 24, 2024
@ti-chi-bot ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Oct 24, 2024
@ti-chi-bot ti-chi-bot
Copy link

ti-chi-bot bot commented Oct 24, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-10-24 11:46:26.209872663 +0000 UTC m=+524386.906663293: ☑️ agreed by lance6716.
  • 2024-10-24 12:41:24.776043911 +0000 UTC m=+527685.472834521: ☑️ agreed by fzzf678.

@Rustin170506
fix: update comments and panic in test
3d31421 
Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>
@Rustin170506
Copy link
Member Author

/unhold

@ti-chi-bot 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 Oct 24, 2024
@lance6716
Copy link
Contributor

/unhold

@Rustin170506
fix: do no handle the nil panic
5910970 
Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>
@Rustin170506
Copy link
Member Author

/retest

@Rustin170506
Copy link
Member Author

/hold

@ti-chi-bot 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 Oct 24, 2024
@Rustin170506
fix: init ctx in OnBecomeOwner
c854877 
Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>
@Rustin170506
fix: use the correct ctx
42b37d9 
Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>
@Rustin170506 Rustin170506 changed the title ddl: add the ddl notifier as a listener to stats owner ddl: add the ddl notifier as a listener to the stats owner Oct 25, 2024
@Rustin170506
Copy link
Member Author

/test unit-test

@tiprow tiprow
Copy link

tiprow bot commented Oct 25, 2024

@Rustin170506: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test fast_test_tiprow
  • /test fast_test_tiprow_ddlargsv1
  • /test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

  • fast_test_tiprow
  • tidb_parser_test

In response to this:

/test unit-test

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
Copy link
Member Author

/unhold

@ti-chi-bot 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 Oct 25, 2024
@ti-chi-bot ti-chi-bot bot merged commit 3ebfad0 into pingcap:master Oct 25, 2024
26 checks passed
@Rustin170506 Rustin170506 deleted the rustin-patch-ddl-stats-owner branch October 25, 2024 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lance6716 lance6716 lance6716 approved these changes

@fzzf678 fzzf678 fzzf678 approved these changes

Assignees
No one assigned
Labels
approved component/ddl This issue is related to DDL of TiDB. lgtm release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Rustin170506 @lance6716 @fzzf678