-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add transaction_timeout session variable
#18560
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
Add transaction_timeout session variable
#18560
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
| @@ -376,6 +376,9 @@ message ExecuteOptions { | |||
|
|
|||
| // in_dml_execution indicates that the query is being executed as part of a DML execution. | |||
| bool in_dml_execution = 19; | |||
|
|
|||
| // transaction_timeout specifies the transaction timeout in milliseconds. If not set, the default timeout is used. | |||
| optional int64 transaction_timeout = 20; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to add this only to ExecuteOptions, although I saw that many fields were both in the overall Session and ExecuteOptions. Query timeout, for example, was in both, but not sure if there's an overall pattern I should be following.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is right place for transaction_timeout.
For query timeout, it can be overriden by query comments so it cannot be part of execute options. As we want to send only authoritative value down after applying the override rules.
Add a `transaction_timeout` session variable. If set, it will be used instead of the value set by `--queryserver-config-transaction-timeout`. Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
8f8bc3a to
ae27b1b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18560 +/- ##
==========================================
+ Coverage 67.49% 67.52% +0.03%
==========================================
Files 1607 1613 +6
Lines 263104 263803 +699
==========================================
+ Hits 177569 178133 +564
- Misses 85535 85670 +135 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Mohamed Hamza <[email protected]>
harshit-gangal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add some e2e test for it.
|
This also require website doc update |
|
Will do! |
Add docs for new `transaction_timeout` system variable introduced in vitessio/vitess#18560.
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Add docs for new `transaction_timeout` system variable introduced in vitessio/vitess#18560. Signed-off-by: Mohamed Hamza <[email protected]>
Description
Add a
transaction_timeoutsession variable. The timeout used will be the smaller value of the session variable or--queryserver-config-transaction-timeout.Related Issue(s)
#18554
Checklist
Deployment Notes