DO NOT MERGE: replica transaction by deepthi · Pull Request #62 · planetscale/vitess

deepthi · 2020-05-22T22:55:06Z

creating a PR to get feedback

go/test/endtoend/tabletgateway/healthcheck/vtgate_test.go

deepthi · 2020-05-22T23:04:08Z

go/vt/srvtopo/resolver.go

@sougou we need a TODO to clean this up. Wha't s the best way to word it?

go/vt/vtgate/executor.go

go/vt/vtgate/tx_conn.go

deepthi · 2020-05-22T23:19:50Z

I removed the checks that were preventing replica transactions in various places. However, I believe we need some other checks to ensure that we are allowing replica transactions only if we have a pinned tablet to route the subsequent queries to.

Signed-off-by: deepthi <deepthi@planetscale.com>

Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: deepthi <deepthi@planetscale.com>

Signed-off-by: deepthi <deepthi@planetscale.com>

Signed-off-by: Andres Taylor <andres@planetscale.com>

Signed-off-by: deepthi <deepthi@planetscale.com>

…ction Signed-off-by: Andres Taylor <andres@planetscale.com>

…orrect tablet Signed-off-by: deepthi <deepthi@planetscale.com>

systay · 2020-05-28T06:56:41Z

go/vt/vtgate/tx_conn.go

+			return err
+		}
+	}
+	// TODO(deepthi): TransactionId should be set to 0 only if the commit succeeded


I don't feel this is correct. If we tried and failed to commit, we should not trust that we still have an open transaction on this connection. I think we should set the tx_id to 0 and close the connection, since we can't trust it

That is valid. I think we got it exactly backwards in the first attempt to fix it.
The way this should work is:

Commit is successful. All shardSession transactions have ended, so set all TransactionID = 0

There was an error from one or more shards. Rollback the other shards. Rollback should only operate on the shards that did not fail. So set TransactionID=0 on failed shards so that we attempt rollback only on successful shards. There is no need to explicitly set TransactionID = 0 on the successful shards at the end of rollback. the defer call to session.Reset takes care of that.

Signed-off-by: Andres Taylor <andres@planetscale.com>

… shards if a subset fails Signed-off-by: deepthi <deepthi@planetscale.com>

Signed-off-by: deepthi <deepthi@planetscale.com>

systay · 2020-05-29T06:58:24Z

go/vt/vtgate/tx_conn_test.go

@@ -132,7 +132,7 @@ func TestTxConnCommitOrderFailure1(t *testing.T) {
 	utils.MustMatch(t, &wantSession, session.Session, "Session")
 	assert.EqualValues(t, 1, sbc0.CommitCount.Get(), "sbc0.CommitCount")
 	// When the commit fails, we try to clean up by issuing a rollback


We should add assertions about sbc1.CommitCount here, and sbc0.RollbackCount in the next test to make it clearer

Added assertions though I'm not sure I understand them :)

systay · 2020-05-29T07:02:26Z

go/vt/vtgate/tx_conn.go

 	}

+	var qs queryservice.QueryService
+	if UsingLegacyGateway() {


We really need to remove the legacy gateway, or add more capabilities to it. these checks are really not good to spread out like this

systay · 2020-05-29T07:04:12Z

go/test/endtoend/tabletgateway/healthcheck/vtgate_test.go

 	qr, err := conn.ExecuteFetch(query, 1000, true)
-	require.Nil(t, err)
+	if expectError == "" {
+		require.Nil(t, err)


When checking for errors, we get better assertion error messages if we use require.NoError instead of require.Nil

+1. I was looking for this, but thought it was something like NilError.

Signed-off-by: Andres Taylor <andres@planetscale.com>

deepthi · 2020-05-29T16:33:54Z

go/vt/vtgate/engine/route.go

 	return shardVars
 }

+func allowOnlyMaster(rss ...*srvtopo.ResolvedShard) error {


Signed-off-by: deepthi <deepthi@planetscale.com>

…nd works Signed-off-by: deepthi <deepthi@planetscale.com>

…st healthcheck Signed-off-by: deepthi <deepthi@planetscale.com>

Signed-off-by: deepthi <deepthi@planetscale.com>

deepthi · 2020-05-29T20:00:23Z

opened vitessio#6244. closing this one.

deepthi requested a review from sougou as a code owner May 22, 2020 22:55

deepthi requested review from harshit-gangal and systay May 22, 2020 23:01

deepthi commented May 22, 2020

View reviewed changes

go/test/endtoend/tabletgateway/healthcheck/vtgate_test.go Outdated Show resolved Hide resolved

deepthi commented May 22, 2020

View reviewed changes

go/vt/srvtopo/resolver.go Outdated

Copy link
Copy Markdown

Author

deepthi May 22, 2020

Choose a reason for hiding this comment

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

@sougou we need a TODO to clean this up. Wha't s the best way to word it?

deepthi commented May 22, 2020

View reviewed changes

go/vt/vtgate/executor.go Outdated Show resolved Hide resolved

deepthi commented May 22, 2020

View reviewed changes

go/vt/vtgate/tx_conn.go Outdated Show resolved Hide resolved

deepthi force-pushed the ds-replica-transaction branch from 903fd9a to fc7c11e Compare May 24, 2020 00:39

deepthi and others added 3 commits May 25, 2020 15:18

replica transactions: initial implementation

99faad4

Signed-off-by: deepthi <deepthi@planetscale.com>

replica transactions: do not allow if using discoverygateway

b97066b

Signed-off-by: deepthi <deepthi@planetscale.com>

replica transactions: fixed endtoend test

65a8a5c

Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: deepthi <deepthi@planetscale.com>

deepthi force-pushed the ds-replica-transaction branch from 2d1aaf1 to 65a8a5c Compare May 25, 2020 22:22

deepthi and others added 5 commits May 25, 2020 15:41

replica transactions: tabletHealthCheck no longer has a mu

2e647fc

Signed-off-by: deepthi <deepthi@planetscale.com>

Cleaned up test

3c14906

Signed-off-by: Andres Taylor <andres@planetscale.com>

Renamed fake test methods to include legacy

8438053

Signed-off-by: Andres Taylor <andres@planetscale.com>

Made TabletGateWay use an interface for the HealthCheck

dccd6b6

Signed-off-by: Andres Taylor <andres@planetscale.com>

Clean up of test

031527c

Signed-off-by: Andres Taylor <andres@planetscale.com>

systay force-pushed the ds-replica-transaction branch 2 times, most recently from 5cf4cba to 031527c Compare May 26, 2020 14:29

systay and others added 2 commits May 26, 2020 21:07

replica transaction: fixed tests

0b7bc39

Signed-off-by: Andres Taylor <andres@planetscale.com>

replica transactions: protect tabletHealthCheck.Conn with mutex

77a274f

Signed-off-by: deepthi <deepthi@planetscale.com>

systay changed the base branch from ds-new-healthcheck to master May 27, 2020 09:49

systay and others added 2 commits May 27, 2020 12:27

Merge remote-tracking branch 'upstream/master' into ds-replica-transa…

abf30b7

…ction Signed-off-by: Andres Taylor <andres@planetscale.com>

replica transactions: commit and rollback should be executed on the c…

72ae94d

…orrect tablet Signed-off-by: deepthi <deepthi@planetscale.com>

systay reviewed May 28, 2020

View reviewed changes

Make tx_engine use read only transactions when in replica mode

be11233

Signed-off-by: Andres Taylor <andres@planetscale.com>

systay force-pushed the ds-replica-transaction branch from ff85a4a to be11233 Compare May 28, 2020 08:20

systay added 2 commits May 28, 2020 15:12

Test cleanup

c5cffda

Signed-off-by: Andres Taylor <andres@planetscale.com>

Update test assertions

ab38d4a

Signed-off-by: Andres Taylor <andres@planetscale.com>

deepthi added 3 commits May 28, 2020 10:46

replica transactions: tx_conn.commitNormal should rollback successful…

c14319a

… shards if a subset fails Signed-off-by: deepthi <deepthi@planetscale.com>

replica transactions: allow with prepared statements, fix commit2PC

015849a

Signed-off-by: deepthi <deepthi@planetscale.com>

replica transactions: disallow DMLs on non-master tablets

a8915b1

Signed-off-by: deepthi <deepthi@planetscale.com>

systay reviewed May 29, 2020

View reviewed changes

systay added 2 commits May 29, 2020 09:50

replica transactions: test cleanup

7900711

Signed-off-by: Andres Taylor <andres@planetscale.com>

replica transactions: refactor check so it's done in one place

f13b506

Signed-off-by: Andres Taylor <andres@planetscale.com>

systay force-pushed the ds-replica-transaction branch from 7a73eab to f13b506 Compare May 29, 2020 07:51

replica-transactions: updated tests to work with the new master checking

e149589

Signed-off-by: Andres Taylor <andres@planetscale.com>

deepthi commented May 29, 2020

View reviewed changes

go/vt/vtgate/engine/route.go

return shardVars

}

func allowOnlyMaster(rss ...*srvtopo.ResolvedShard) error {

Copy link
Copy Markdown

Author

deepthi May 29, 2020

Choose a reason for hiding this comment

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

+1

deepthi added 4 commits May 29, 2020 12:19

test framework: rename funcs, replica tablet type should be set

a79abb3

Signed-off-by: deepthi <deepthi@planetscale.com>

replica transaction: test tablet down and up produces correct error a…

bb7ac34

…nd works Signed-off-by: deepthi <deepthi@planetscale.com>

replica transactions: move test up one level since it is no longer ju…

bc2d19b

…st healthcheck Signed-off-by: deepthi <deepthi@planetscale.com>

add more assertions to commit order tests

8caab43

Signed-off-by: deepthi <deepthi@planetscale.com>

deepthi closed this May 29, 2020

Conversation

deepthi commented May 22, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

deepthi commented May 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deepthi May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

systay May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deepthi commented May 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

deepthi May 28, 2020 •

edited

Loading

systay May 29, 2020 •

edited

Loading