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.

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

executor: add rollback in releaseSysSession #14269

Merged
merged 2 commits into from
Dec 27, 2019
Merged

executor: add rollback in releaseSysSession #14269

merged 2 commits into from
Dec 27, 2019

Conversation

lysu
Copy link
Collaborator

@lysu lysu commented Dec 27, 2019

What problem does this PR solve?

in #11201 we add releaseSysSession method to reuse transaction restrict session.

but it have potential to release a uncommit session to sessionPool

What is changed and how it works?

force rollback session before release to session pool.
close session directly if session rollback failed.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • add a rollback call

Side effects

  • n/a

Related changes

  • cherry-pick to 3.0

Release note

  • n/a

This change is Reviewable

@lysu lysu added type/bug-fix This PR fixes a bug. component/server labels Dec 27, 2019
@lysu lysu requested review from coocood and jackysp December 27, 2019 13:23
@lysu lysu added needs-cherry-pick-3.0 priority/release-blocker This PR blocks a release. Please review it ASAP. labels Dec 27, 2019
@lysu
Copy link
Collaborator Author

lysu commented Dec 27, 2019

/run-all-tests

@lysu lysu added Priority/P1 Features that will be implemented in the latest or next major/minor version status/all tests passed labels Dec 27, 2019
@tiancaiamao
Copy link
Contributor

Do you know which internal session fails to call commit/rollback? @lysu

@lysu
Copy link
Collaborator Author

lysu commented Dec 27, 2019

@tiancaiamao for example this line

return err

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp jackysp added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 27, 2019
@jackysp
Copy link
Member

jackysp commented Dec 27, 2019

Tests

Unit test
Integration test

What's the integration test looks like? Concurrently drop an unknown user?

Copy link
Member

@coocood coocood left a comment

Choose a reason for hiding this comment

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

LGTM

@coocood coocood merged commit c6cb405 into pingcap:master Dec 27, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 27, 2019

cherry pick to release-3.0 in PR #14272

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/server Priority/P1 Features that will be implemented in the latest or next major/minor version priority/release-blocker This PR blocks a release. Please review it ASAP. status/LGT1 Indicates that a PR has LGTM 1. type/bug-fix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants