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

sessionctx: Error/warning on unsupported isolation levels #8625

Merged
merged 8 commits into from
Dec 10, 2018
Merged

sessionctx: Error/warning on unsupported isolation levels #8625

merged 8 commits into from
Dec 10, 2018

Conversation

morgo
Copy link
Contributor

@morgo morgo commented Dec 8, 2018

What problem does this PR solve?

Fixes #2712

What is changed and how it works?

For isolation levels, the following behavior is provided:

  • read-uncommitted: error (unsupported)
  • read-committed: clean
  • repeatable-read: clean
  • serializable: error (unsupported)

Check List

Tests

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

Code changes

  • minimal

Side effects

  • Breaking backward compatibility (there may be some apps which depend on read-uncommitted or serializable, but they are less common than RR and RC)

Related changes

  • Need to update the documentation
  • Need to be included in the release note

This change is Reviewable

@morgo
Copy link
Contributor Author

morgo commented Dec 8, 2018

/run-all-tests

@zhouqiang-cl
Copy link
Contributor

/rebuild

@morgo
Copy link
Contributor Author

morgo commented Dec 9, 2018

I have changed read-uncommitted to be an error now, and updated the description. read-committed also uses a new error class of NotRecommended.

@zhouqiang-cl
Copy link
Contributor

/run-all-tests

@morgo
Copy link
Contributor Author

morgo commented Dec 9, 2018

It looks like this does break the JDBC test:

Loading JDBC driver 'com.mysql.jdbc.Driver'

Connected to 5.7.10-TiDB-v2.1.0-rc.3-299-g0dc57f7

[ERROR] Tests run: 30, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 4.957 s <<< FAILURE! - in pingcap.tidb.test.simple.ConnectionTest

[ERROR] testIsolationLevel(pingcap.tidb.test.simple.ConnectionTest)  Time elapsed: 0.01 s  <<< ERROR!

java.sql.SQLException: variable 'tx_isolation' does not yet support value: READ-UNCOMMITTED

	at pingcap.tidb.test.simple.ConnectionTest.testIsolationLevel(ConnectionTest.java:553)

This is the side-effect mentioned in the PR. Some applications will break on warnings too, particularly since read-committed is frequently used.

@morgo
Copy link
Contributor Author

morgo commented Dec 9, 2018

/run-all-tests

@morgo
Copy link
Contributor Author

morgo commented Dec 9, 2018

I was concerned that a warning for read-committed may break too many applications, so I've changed it to be clean again. The description has been updated.

@morgo
Copy link
Contributor Author

morgo commented Dec 9, 2018

/run-all-tests tidb-test=pr/683

@disksing
Copy link
Contributor

LGTM.

@zz-jason zz-jason added status/LGT1 Indicates that a PR has LGTM 1. component/server labels Dec 10, 2018
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@winkyao winkyao added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 10, 2018
@ngaut
Copy link
Member

ngaut commented Dec 10, 2018

/run-all-tests

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/server contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What transaction isolation is supported?
7 participants