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

*: clean up the deserted syntax of START TRANSACTION READ ONLY #24813

Merged
merged 13 commits into from
May 25, 2021
Merged

*: clean up the deserted syntax of START TRANSACTION READ ONLY #24813

merged 13 commits into from
May 25, 2021

Conversation

JmPotato
Copy link
Member

@JmPotato JmPotato commented May 21, 2021 •
edited
Loading

Signed-off-by: JmPotato ghzpotato@gmail.com

What problem does this PR solve?

Issue Number: close #24291.

Problem Summary: After supporting the AS OF TIMESTAMP syntax for START TRANSACTION READ ONLY statement, we need to clean up the remaining deserted syntax.

What is changed and how it works?

  • Remove sessionctx.StalenessTxnOption since we don't need it anymore.
  • Change NewTxnWithStalenessOption to NewTxnWithStartTS.
  • Remove the useless fields from StartTSOption.
  • Remove the unnecessary test cases.
  • Update the test cases.

Check List

Tests

  • Unit test
  • Integration test

Release note

  • No release note.

@JmPotato
Clean up the deserted syntax of START TRANSACTION READ ONLY
97bce65 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato JmPotato requested review from a team as code owners May 21, 2021 04:03
@JmPotato JmPotato requested review from qw4990 and removed request for a team May 21, 2021 04:03
@ti-chi-bot ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 21, 2021
@github-actions github-actions bot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels May 21, 2021
@JmPotato
Fix the check
05a14fc 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato
Copy link
Member Author

/cc @xhebox @Yisaer @nolouch @djshow832

@JmPotato
Add more test cases
0a55fac 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@ti-chi-bot ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 21, 2021
@JmPotato JmPotato mentioned this pull request May 23, 2021
Merged
@JmPotato JmPotato mentioned this pull request May 24, 2021
Merged
Yisaer
Yisaer reviewed May 24, 2021
View reviewed changes
Copy link
Contributor

@Yisaer Yisaer left a comment

Choose a reason for hiding this comment

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

Rest LGTM

executor/simple.go Outdated Show resolved Hide resolved
store/tikv/tests/extract_start_ts_test.go Outdated Show resolved Hide resolved
@JmPotato
Remove the ExtractStartTS test
8352d99 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato
Copy link
Member Author

CI tests will pass after pingcap/br#1139 is merged.

@JmPotato
Merge branch 'master' of github.com:pingcap/tidb into clean_up_start_…
3c62707 
…transaction_executor
@JmPotato
Fix a typo
b53b32a 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato
Update the BR version
c3abc48 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato
Copy link
Member Author

/run-all-tests

@xhebox
Copy link
Contributor

xhebox commented May 25, 2021

/lgtm

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 25, 2021
Yisaer
Yisaer approved these changes May 25, 2021
View reviewed changes
Copy link
Contributor

@Yisaer Yisaer left a comment

Choose a reason for hiding this comment

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

/lgtm

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • Yisaer
  • xhebox

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 25, 2021
@Yisaer
Copy link
Contributor

Yisaer commented May 25, 2021

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 6c41d3e

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 25, 2021
@JmPotato
Copy link
Member Author

/run-e2e-test

@ti-chi-bot ti-chi-bot merged commit 98f0d76 into pingcap:master May 25, 2021
@JmPotato JmPotato deleted the clean_up_start_transaction_executor branch May 25, 2021 06:25
@dveeden dveeden mentioned this pull request May 27, 2021
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yisaer Yisaer Yisaer approved these changes

@qw4990 qw4990 Awaiting requested review from qw4990

@djshow832 djshow832 Awaiting requested review from djshow832

@nolouch nolouch Awaiting requested review from nolouch

@xhebox xhebox Awaiting requested review from xhebox

Assignees
No one assigned
Labels
sig/execution SIG execution sig/sql-infra SIG: SQL Infra size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace the WITH Staleness to AS OF clause
4 participants
@JmPotato @xhebox @ti-chi-bot @Yisaer