Add conclude transaction command to vtctld service #16693

harshit-gangal · 2024-09-02T10:05:46Z

Description

This PR adds support for vtctld server and vtctld client to Conclude open distributed transactions.
It also changes the command usage for Distributed Transactions

Command Usage
List:
DistributedTransaction list <keyspace>
Conclude:
DistributedTransaction conclude <dtid> [<keyspace/shard> ...]

Related Issue(s)

RFC: Atomic Distributed Transactions #16245

Checklist

Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

vitess-bot · 2024-09-02T10:05:48Z

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

Ensure that the Pull Request has a descriptive title.
Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

Apply the release notes (needs details) label if users need to know about this change.
New features should be documented.
There should be some code comments as to why things are implemented the way they are.
There should be a comment at the top of each new or modified test to explain what the test does.

New flags

Is this flag really necessary?
Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

Each item in Jobs should be named in order to mark it as required.
If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

Protobuf changes should be wire-compatible.
Changes to _vt tables and RPCs need to be backward compatible.
RPC changes should be compatible with vitess-operator
If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
vtctl command output order should be stable and awk-able.

codecov · 2024-09-02T10:26:20Z

Codecov Report

Attention: Patch coverage is 39.53488% with 78 lines in your changes missing coverage. Please review.

Project coverage is 68.93%. Comparing base (fd18ae4) to head (3ed6b66).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
go/cmd/vtctldclient/command/transactions.go	8.88%	41 Missing ⚠️
go/vt/vttablet/grpctmclient/client.go	0.00%	10 Missing ⚠️
go/vt/vttablet/grpctmserver/server.go	0.00%	7 Missing ⚠️
go/vt/vtctl/grpcvtctldserver/server.go	84.84%	5 Missing ⚠️
go/vt/vtctl/grpcvtctldclient/client_gen.go	0.00%	4 Missing ⚠️
go/vt/vttablet/tmrpctest/test_tm_rpc.go	0.00%	4 Missing ⚠️
go/vt/vtcombo/tablet_map.go	0.00%	2 Missing ⚠️
go/vt/vtctl/localvtctldclient/client_gen.go	0.00%	2 Missing ⚠️
go/vt/vttablet/faketmclient/fake_client.go	0.00%	2 Missing ⚠️
go/vt/vttablet/tabletmanager/rpc_query.go	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16693      +/-   ##
==========================================
- Coverage   68.93%   68.93%   -0.01%     
==========================================
  Files        1565     1566       +1     
  Lines      201741   201855     +114     
==========================================
+ Hits       139076   139152      +76     
- Misses      62665    62703      +38

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

go/cmd/vtctldclient/command/query.go

rohit-nayak-ps · 2024-09-04T17:26:25Z

Instead of the two current commands GetUnresolvedTransactions and ConcludeTransaction what do you think about a single umbrella command similar to those we have in vreplication like https://vitess.io/docs/20.0/reference/programs/vtctldclient/vtctldclient_workflow/.

Like
DistributedTransactions -k <keyspace> Show —unresolved // --unresolved could be default
DistributedTransactions -k <keyspace> Conclude [—shards <shard1,shard2>] —dtid <dtid>

The advantage of this approach is that we minimize the new commands required for additional functionality like (just brainstorming here)
DistributedTransactions -k <keyspace> Show —pending

Not sure if it is an overkill here and we will not have any more operations for DTs, but thought I would mention it.

harshit-gangal · 2024-09-10T14:22:35Z

Instead of the two current commands GetUnresolvedTransactions and ConcludeTransaction what do you think about a single umbrella command similar to those we have in vreplication like https://vitess.io/docs/20.0/reference/programs/vtctldclient/vtctldclient_workflow/.

Like DistributedTransactions -k <keyspace> Show —unresolved // --unresolved could be default DistributedTransactions -k <keyspace> Conclude [—shards <shard1,shard2>] —dtid <dtid>

The advantage of this approach is that we minimize the new commands required for additional functionality like (just brainstorming here) DistributedTransactions -k <keyspace> Show —pending

Not sure if it is an overkill here and we will not have any more operations for DTs, but thought I would mention it.

This is a reasonable ask, at the moment we know of 2 commands but it can grow over time.
It will ensure that commands are tied together.

I will make the required changes.

…ager Signed-off-by: Harshit Gangal <harshit@planetscale.com>

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

deepthi · 2024-09-11T15:17:35Z

go/cmd/vtctldclient/command/transactions.go

+ // ConcludeTransaction makes a ConcludeTransaction gRPC call to a vtctld.
+ ConcludeTransaction = &cobra.Command{
+ Use: "conclude <dtid> [<keyspace/shard> ...]",
+ Short: "Concludes the unresolved transaction by rolling back the prepared transaction on all participating shards and removing the transaction metadata record.",


Suggested change

Short: "Concludes the unresolved transaction by rolling back the prepared transaction on all participating shards and removing the transaction metadata record.",

Short: "Concludes the unresolved transaction by rolling back prepared transactions on all participating shards and removing the transaction metadata record.",

every shard will have at most 1 prepared transaction.

I believe it is how we see this distributed transaction.
one transaction that spans multiple shards
or multiple independent transactions on the shards.

For user, it is one transaction.
Internally it is multiple transactions.

if you want to use singular, you can use each

Concludes the unresolved transaction by rolling back the prepared transaction on each participating shard and removing the transaction metadata record

OR

Concludes the unresolved transaction by rolling back prepared transactions on all participating shards and removing the transaction metadata record

are both grammatically correct. Using the singular with "all" is not. Ask ChatGPT and you will see.

I will take the first one

@deepthi could you also approve the PR if everything else looks good?

rohit-nayak-ps · 2024-09-11T15:00:41Z

go/flags/endtoend/vtctldclient.txt

@@ -28,6 +28,7 @@ Available Commands:
 DeleteShards Deletes the specified shards from the topology.
 DeleteSrvVSchema Deletes the SrvVSchema object in the given cell.
 DeleteTablets Deletes tablet(s) from the topology.
+ DistributedTransaction Perform commands on distributed transaction


Should this be plural? I see from the discussion below that there is just one dt per shard: is that for all users and connections?

Plural makes more sense to me too, especially for DistributedTransactions list since there can be multiple transactions that are listed.

rohit-nayak-ps · 2024-09-11T18:22:20Z

go/cmd/vtctldclient/command/transactions.go

+ Use: "conclude <dtid> [<keyspace/shard> ...]",
+ Short: "Concludes the unresolved transaction by rolling back the prepared transaction on all participating shards and removing the transaction metadata record.",
+ Aliases: []string{"Conclude"},
+ Args: cobra.MinimumNArgs(1),


From the examples it is not entirely clear to me what the arguments to conclude will look like. But we have found it more useful to specify the parameters as flags rather than depend on args because it is easy to forget the order or delimiters etc. We had that problem a lot with the earlier vreplication commands. It is a bit more typing

For example it could be
DistributedTransactions Conclude --keyspace ks1 --shards -40,a0- --dtid 82345 OR with shorthand codes

DistributedTransactions Conclude -k ks1 -s -40,a0- --dtid 82345

You can decide if this works better or not ...

A transaction can span multiple keyspaces so keyspace and shard will go in pair like keyspace/shard

I did not find a good way to record the participants list.
We keep it like this for now.

go/vt/vtctl/grpcvtctldserver/server.go

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

…nsactions Signed-off-by: Harshit Gangal <harshit@planetscale.com>

GuptaManan100

Rest LGTM

GuptaManan100 · 2024-09-16T05:23:54Z

go/cmd/vtctldclient/command/transactions.go

+
+ // ConcludeTransaction makes a ConcludeTransaction gRPC call to a vtctld.
+ ConcludeTransaction = &cobra.Command{
+ Use: "conclude <dtid> [<keyspace/shard> ...]",


Should we even take in <keyspace/shard> list? Why don't we just conclude the transaction everywhere. Like the user only has one option, which is to conclude the transaction on all the shards, not on some shards.

I am thinking about it that if the list of keyspace/shard is not provided we will get it and then conclude on all.
This is more from if the user gets the unresolved transactions list then it knows which all shards are involved and can provide the list to conclude.

We can make the flow better. Let's do that in a follow up PR.

GuptaManan100 · 2024-09-16T05:25:14Z

go/vt/vtctl/grpcvtctldserver/server.go

+ for _, rm := range req.Participants {
+ eg.Go(func() error {
+ primary, err := s.getPrimaryTablet(newCtx, rm)
+ if err != nil {
+ return err
+ }
+ return s.tmc.ConcludeTransaction(newCtx, primary.Tablet, req.Dtid, false)
+ })
+ }


What if the ss.Target tablet is part of the requested participants. Would we not end up calling ConcludeTransaction on it twice? First with the last argument being false, and the next time with true.

Also, if the ss.Target is not part of the requested participants, should we even do the second ConcludeTransaction call?

That is the Metadata Manager Shard, it's conclude is about deleting the Transaction record once everything is resolved in other participating shards.

Participating shards is exclusive of Metadata Manager shard.

github-actions bot added this to the v21.0.0 milestone Sep 2, 2024

harshit-gangal force-pushed the vtctld-conclude-transactions branch 2 times, most recently from 048e39c to ca4c140 Compare September 3, 2024 10:06

harshit-gangal removed the Skip CI Skip CI actions from running label Sep 3, 2024

harshit-gangal force-pushed the vtctld-conclude-transactions branch from 84c5238 to 73f1a84 Compare September 4, 2024 16:49

harshit-gangal marked this pull request as ready for review September 4, 2024 16:53

harshit-gangal requested review from deepthi, mattlord, ajm188, notfelineit, GuptaManan100, rohit-nayak-ps, shlomi-noach, systay and timvaillancourt as code owners September 4, 2024 16:53

deepthi reviewed Sep 4, 2024

View reviewed changes

go/cmd/vtctldclient/command/query.go Outdated Show resolved Hide resolved

harshit-gangal marked this pull request as draft September 10, 2024 15:32

harshit-gangal removed request for timvaillancourt, ajm188 and notfelineit September 10, 2024 15:32

harshit-gangal marked this pull request as ready for review September 11, 2024 14:48

harshit-gangal added 7 commits September 11, 2024 20:20

feat: implementation of Conclude Transaction in vtctld and tablet man…

aaf21e4

…ager Signed-off-by: Harshit Gangal <harshit@planetscale.com>

feat: added ConcludeTransaction command to vtcltdclient

3c8dee3

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

test: added test cases in different packages

78d55dc

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

use original context for mm shard conclude and add command test

69f2794

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

refactor: make DistributedTransaction as umbrella command

dbdb010

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

test: update flag test output

89d22e3

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

add json output to Conclude command

5facda3

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

harshit-gangal force-pushed the vtctld-conclude-transactions branch from b79484d to 5facda3 Compare September 11, 2024 14:51

deepthi reviewed Sep 11, 2024

View reviewed changes

rohit-nayak-ps approved these changes Sep 11, 2024

View reviewed changes

add span annotates

52e191b

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

harshit-gangal mentioned this pull request Sep 13, 2024

RFC: Atomic Distributed Transactions #16245

Open

17 tasks

harshit-gangal added 2 commits September 13, 2024 14:12

address review comments

bb38600

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

Merge remote-tracking branch 'upstream/main' into vtctld-conclude-tra…

3ed6b66

…nsactions Signed-off-by: Harshit Gangal <harshit@planetscale.com>

GuptaManan100 approved these changes Sep 16, 2024

View reviewed changes

harshit-gangal merged commit 8b57e7c into vitessio:main Sep 16, 2024
100 checks passed

harshit-gangal deleted the vtctld-conclude-transactions branch September 16, 2024 05:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conclude transaction command to vtctld service #16693

Add conclude transaction command to vtctld service #16693

harshit-gangal commented Sep 2, 2024 •

edited

Loading

vitess-bot bot commented Sep 2, 2024

codecov bot commented Sep 2, 2024 •

edited

Loading

rohit-nayak-ps commented Sep 4, 2024

harshit-gangal commented Sep 10, 2024

deepthi Sep 11, 2024

harshit-gangal Sep 11, 2024

harshit-gangal Sep 11, 2024

deepthi Sep 12, 2024 •

edited

Loading

harshit-gangal Sep 13, 2024

harshit-gangal Sep 13, 2024

rohit-nayak-ps Sep 11, 2024

GuptaManan100 Sep 16, 2024

rohit-nayak-ps Sep 11, 2024

harshit-gangal Sep 12, 2024

harshit-gangal Sep 12, 2024

GuptaManan100 left a comment

GuptaManan100 Sep 16, 2024

harshit-gangal Sep 16, 2024

GuptaManan100 Sep 16, 2024

harshit-gangal Sep 16, 2024

	Short: "Concludes the unresolved transaction by rolling back the prepared transaction on all participating shards and removing the transaction metadata record.",
	Short: "Concludes the unresolved transaction by rolling back prepared transactions on all participating shards and removing the transaction metadata record.",

Add conclude transaction command to vtctld service #16693

Add conclude transaction command to vtctld service #16693

Conversation

harshit-gangal commented Sep 2, 2024 • edited Loading

Description

Related Issue(s)

Checklist

vitess-bot bot commented Sep 2, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Sep 2, 2024 • edited Loading

Codecov Report

rohit-nayak-ps commented Sep 4, 2024

harshit-gangal commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepthi Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuptaManan100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshit-gangal commented Sep 2, 2024 •

edited

Loading

codecov bot commented Sep 2, 2024 •

edited

Loading

deepthi Sep 12, 2024 •

edited

Loading