-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
decouple olap tx timeout from oltp tx timeout #10946
decouple olap tx timeout from oltp tx timeout #10946
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
b6d5c70
to
a7801e5
Compare
@maxenglander we'll get this reviewed. Can you add notes to the 15_0_0_summary.md file and resolve the conflicts in the meantime? |
27c7708
to
b908164
Compare
5f9ad61
to
98b3941
Compare
…t, +sc.expiryTime Signed-off-by: Max Englander <max@planetscale.com>
…or expiryTime Signed-off-by: Max Englander <max@planetscale.com>
Signed-off-by: Max Englander <max@planetscale.com>
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.
Other things mostly looks good.
I think we should add more tests for Idle transaction getting killed by transaction killer and expiry time getting updated when we swtich workload.
We might have those tests already, just check atleast.
@@ -141,7 +148,7 @@ func (tp *TxPool) transactionKiller() { | |||
if conn.IsInTransaction() { | |||
tp.txComplete(conn, tx.TxKill) | |||
} | |||
conn.Releasef("exceeded timeout: %v", tp.Timeout()) | |||
conn.Releasef("exceeded timeout: %v", timeout) |
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.
nit: can use conn.timeout
directly here in the method.
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.
Oops sorry I didn't implement this suggestion earlier. I saw it, but I misunderstood it 🤦
…fix comments, set ticks interval once Signed-off-by: Max Englander <max@planetscale.com>
f083308
to
4885ab7
Compare
Signed-off-by: Max Englander <max@planetscale.com>
4885ab7
to
17e7b7f
Compare
Signed-off-by: Max Englander <max@planetscale.com>
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 think few previous comments are still pending to be addressed.
Once, addressed can be merged.
Signed-off-by: Max Englander <max@planetscale.com>
Signed-off-by: Max Englander <max@planetscale.com>
Signed-off-by: Max Englander <max@planetscale.com>
@harshit-gangal I implemented the one outstanding suggestion I found ( |
Description
Since
workload=olap
bypasses the query timeouts(
--queryserver-config-query-timeout
) and also row limits, the naturalassumption is that it also bypasses the transaction timeout.
This is not the case, e.g. for a tablet where the
--queryserver-config-transaction-timeout
is 10.This PR
timeouts for OLAP workloads
--queryserver-config-olap-transaction-timeout
with a default value of0
seconds, disabling OLAP TX timeouts.Decouples TX kill interval from OLTP TX timeout via new CLI flag andYAML field
--queryserver-config-transaction-killer-interval
defaulting to3
seconds.One subtlety is that the timeout that is applied to the transaction is based on the value of the workload setting at the beginning of the transaction. If the workload is changed mid-transaction, that may change the timeout applied to queries within the transaction, but it won't change the transaction timeout.
Demo
Using (new) default values), connected to VTGate.
Breaking changes
Currently OLAP transactions are killed after--queryserver-config-transaction-timeout
seconds. With this PR, OLAP transactions are killed after--queryserver-config-olap-transaction-timeout
seconds (default value0
means transactions are not timed out).Currently OLTP and OLAP transactions are evaluated for killing every--queryserver-config-transaction-timeout
seconds divided by10
. With this PR, OLAP and OLTP transactions are evaluated for killing every--queryserver-config-transaction-killer-interval
seconds.Related Issue(s)
#10945
Checklist
Deployment Notes