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

stmtctx, context: remove the redundant implementation of warnings/extraWarnings in statement context #52862

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

stmtctx, context: remove the redundant implementation of warnings/extraWarnings in statement context #52862

wants to merge 1 commit into from
+171 −131

Conversation

YangKeao
Copy link
Member

What problem does this PR solve?

Issue Number: close #52848

Problem Summary:

We have multiple implementation of WarnHandler: the most used one is the StatementContext itself. However, we already have a StaticWarnHandler, which implements nearly all methods needed by the WarnHandler. Now, it'd be better to implement the warning related methods in StatementContext with StaticWarnHandler.

What changed and how does it work?

  1. Remove the warnings and extraWarnings in the StatementContext.
  2. Add two WarnHandlers to handle normal warning and extra warning (which is used in slow log).

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.
      The change should be covered by existing tests

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

None

@ti-chi-bot ti-chi-bot bot added release-note-none size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. sig/planner SIG: Planner labels Apr 24, 2024
@YangKeao YangKeao force-pushed the fix-52848 branch 2 times, most recently from 6c43122 to 8321d26 Compare April 24, 2024 06:27
@codecov Codecov
Copy link

codecov bot commented Apr 24, 2024 •
edited

Codecov Report

Attention: Patch coverage is 98.87640% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 55.9003%. Comparing base (cf5c68e) to head (23f957c).
Report is 82 commits behind head on master.

Additional details and impacted files 
@@                Coverage Diff                @@
##             master     #52862         +/-   ##
=================================================
- Coverage   72.4170%   55.9003%   -16.5168%     
=================================================
  Files          1477       1595        +118     
  Lines        427989     605204     +177215     
=================================================
+ Hits         309937     338311      +28374     
- Misses        98854     243675     +144821     
- Partials      19198      23218       +4020
Flag Coverage Δ
integration 36.8083% <87.6404%> (?)
unit 71.2321% <98.8764%> (-0.0724%) ⬇️

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

Components Coverage Δ
dumpling 53.9957% <ø> (ø)
parser ∅ <ø> (∅)
br 49.2817% <ø> (+8.0690%) ⬆️

@YangKeao
Copy link
Member Author

/retest

@YangKeao YangKeao force-pushed the fix-52848 branch 3 times, most recently from 9d5f000 to f298dc1 Compare April 24, 2024 07:41
YangKeao
YangKeao commented Apr 24, 2024
View reviewed changes
pkg/sessionctx/stmtctx/stmtctx.go
@@ -1017,6 +957,8 @@ func (sc *StatementContext) ResetForRetry() {
sc.IndexNames = sc.IndexNames[:0]
sc.TaskID = AllocateTaskID()
sc.SyncExecDetails.Reset()
sc.WarnHandler.TruncateWarnings(0)
sc.ExtraWarnHandler.TruncateWarnings(0)
Copy link
Member Author

Choose a reason for hiding this comment

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

I've confirmed with @time-and-fate that it's expected to also reset extra warning here. It's a mistake that it wasn't reset in resetMuForRetry before this PR.

@YangKeao YangKeao requested review from lcwangchao, time-and-fate, XuHuaiyu and zimulala and removed request for zimulala April 24, 2024 08:32
@YangKeao
Copy link
Member Author

/retest

@YangKeao
remove the redundant implementation of warnings/extraWarnings in stat…
23f957c 
…ement context

Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
@YangKeao
Copy link
Member Author

/retest

2 similar comments
@YangKeao
Copy link
Member Author

/retest

@YangKeao
Copy link
Member Author

/retest

@ti-chi-bot ti-chi-bot
Copy link

ti-chi-bot bot commented May 8, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lcwangchao
Once this PR has been reviewed and has the lgtm label, please assign hi-rustin, xuhuaiyu for approval, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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
Copy link

ti-chi-bot bot commented May 8, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-05-08 08:05:00.789484997 +0000 UTC m=+1035654.546620564: ☑️ agreed by lcwangchao.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcwangchao lcwangchao lcwangchao approved these changes

@time-and-fate time-and-fate Awaiting requested review from time-and-fate

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

Assignees
No one assigned
Labels
needs-1-more-lgtm release-note-none sig/planner SIG: Planner size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add AppendExtraWarning to the WarnHandler.
2 participants
@YangKeao @lcwangchao