-
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
Fix for reserved connection usage with transaction #7646
Fix for reserved connection usage with transaction #7646
Conversation
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
1. Update the shard sessions only if there is any update i.e. updated transaction id or updated reserved id 2. If there is failure to execute BeginExecute api and the connection is reserved conn then check if shard session can be reset and execute ReserveBeginExecute api Signed-off-by: Harshit Gangal <harshit@planetscale.com>
…erve connection Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@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.
LGTM. It looks like you audited the code and made sure that all paths that return an error do the right thing. Can you confirm that?
exec(t, conn, `insert into allDefaults () values ()`) | ||
exec(t, conn, `commit`) | ||
|
||
time.Sleep(6 * time.Second) |
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 don't suppose this can be made any faster?
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.
transaction timeout is 3 seconds and this is to ensure that transaction killer is run.
@@ -447,7 +447,7 @@ func (sbc *SandboxConn) HandlePanic(err *error) { | |||
//ReserveBeginExecute implements the QueryService interface | |||
func (sbc *SandboxConn) ReserveBeginExecute(ctx context.Context, target *querypb.Target, preQueries []string, sql string, bindVariables map[string]*querypb.BindVariable, options *querypb.ExecuteOptions) (*sqltypes.Result, int64, int64, *topodatapb.TabletAlias, error) { | |||
reservedID := sbc.reserve(ctx, target, preQueries, bindVariables, 0, options) | |||
result, transactionID, alias, err := sbc.BeginExecute(ctx, target, preQueries, sql, bindVariables, reservedID, options) | |||
result, transactionID, alias, err := sbc.BeginExecute(ctx, target, nil, sql, bindVariables, reservedID, options) |
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.
Why was this change needed?
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 was wrong test setup, so fixed it.
prequeries were getting logged twice which was wrong.
For searchability purposes; since there isn't an issue associated with this, here is the type of error that this PR addresses:
|
Description
The issue is when the session is in reserved connection and the connection is timeout.
Next time there is a transaction, that failed with a weird error of tablet alias not matching.
The fix has three parts.
Checklist
Impacted Areas in Vitess
Components that this PR will affect: