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

context: Add method to detach the tableReaderExecutor from the session context #53337

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

context: Add method to detach the tableReaderExecutor from the session context #53337

wants to merge 1 commit into from

Conversation

YangKeao
Copy link
Member

What problem does this PR solve?

Issue Number: close #53336

Problem Summary:

It's not safe to call Next (or other method) of an executor after the session is executing the next statement. We have split out a lot of Context and it's clear that the context depended by the TableReaderExecutor is safe to be re-used (after proper configuration) while executing another statement is running on the session.

What changed and how does it work?

This PR adds IntoStatic method for several contexts. After calling this method, the context can be safe to be used while the session continues to run the next statements.

Check List

Tests

TODO:

  • 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
Copy link

ti-chi-bot bot commented May 16, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-tests-checked do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none labels May 16, 2024
@YangKeao
Copy link
Member Author

/retest

@ti-chi-bot ti-chi-bot bot added sig/planner SIG: Planner size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 16, 2024
@tiprow tiprow
Copy link

tiprow bot commented May 16, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@YangKeao YangKeao force-pushed the clone-table-reader-executor branch from 82d36eb to cec996c Compare May 17, 2024 05:46
@YangKeao
Copy link
Member Author

/retest

@YangKeao YangKeao force-pushed the clone-table-reader-executor branch from cec996c to c106156 Compare May 17, 2024 05:53
@YangKeao
Copy link
Member Author

/retest

@codecov Codecov
Copy link

codecov bot commented May 17, 2024 •
edited

Codecov Report

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

Project coverage is 74.6350%. Comparing base (0afe54d) to head (740ccd3).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@               Coverage Diff                @@
##             master     #53337        +/-   ##
================================================
+ Coverage   72.5179%   74.6350%   +2.1171%     
================================================
  Files          1505       1506         +1     
  Lines        429866     429972       +106     
================================================
+ Hits         311730     320910      +9180     
+ Misses        98820      89222      -9598     
- Partials      19316      19840       +524
Flag Coverage Δ
integration 49.1553% <15.1515%> (?)

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

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

@YangKeao YangKeao force-pushed the clone-table-reader-executor branch from c106156 to 52933d6 Compare May 22, 2024 04:44
@YangKeao
Copy link
Member Author

/retest

@YangKeao
Add method to detach the tableReaderExecutor from the session context
740ccd3 
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
@YangKeao YangKeao force-pushed the clone-table-reader-executor branch from 52933d6 to 740ccd3 Compare May 22, 2024 05:10
@ti-chi-bot ti-chi-bot
Copy link

ti-chi-bot bot commented May 22, 2024

[FORMAT CHECKER NOTIFICATION]

Notice: To remove the do-not-merge/needs-tests-checked label, please finished the tests then check the finished items in description.

For example:

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

📖 For more info, you can check the "Contribute Code" section in the development guide.

@YangKeao
Copy link
Member Author

/retest

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

ti-chi-bot bot commented May 22, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign windtalker, winoros, zimulala 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

@tiprow tiprow
Copy link

tiprow bot commented May 22, 2024

@YangKeao: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
fast_test_tiprow 740ccd3 link true /test fast_test_tiprow

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

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

ti-chi-bot bot commented May 22, 2024 •
edited

@YangKeao: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
idc-jenkins-ci-tidb/unit-test 740ccd3 link true /test unit-test

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
do-not-merge/needs-tests-checked do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 a method to detach the context used by TableReaderExecutor
1 participant
@YangKeao