From ae06dd19c0a281bf277c1b665ec7e0e04cebc97b Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 16 Sep 2024 19:51:57 +0530 Subject: [PATCH] fix: empty transaction metadata should short circuit the conclude call Signed-off-by: Harshit Gangal --- go/vt/vtctl/grpcvtctldserver/server.go | 3 +- go/vt/vtctl/grpcvtctldserver/server_test.go | 67 +++++++++++-------- .../testutil/test_tmclient.go | 21 +++--- 3 files changed, 55 insertions(+), 36 deletions(-) diff --git a/go/vt/vtctl/grpcvtctldserver/server.go b/go/vt/vtctl/grpcvtctldserver/server.go index dc0013f1117..430453fd23b 100644 --- a/go/vt/vtctl/grpcvtctldserver/server.go +++ b/go/vt/vtctl/grpcvtctldserver/server.go @@ -2462,7 +2462,8 @@ func (s *VtctldServer) ConcludeTransaction(ctx context.Context, req *vtctldatapb if len(participants) == 0 { // Read the transaction metadata if participating resource manager list is not provided in the request. transaction, err := s.tmc.ReadTransaction(ctx, primary.Tablet, req.Dtid) - if err != nil { + if transaction == nil || err != nil { + // no transaction record for the given ID. It is already concluded or does not exist. return nil, err } participants = transaction.Participants diff --git a/go/vt/vtctl/grpcvtctldserver/server_test.go b/go/vt/vtctl/grpcvtctldserver/server_test.go index 5dd627e6ea1..adc4095fd06 100644 --- a/go/vt/vtctl/grpcvtctldserver/server_test.go +++ b/go/vt/vtctl/grpcvtctldserver/server_test.go @@ -5257,34 +5257,47 @@ func TestConcludeTransaction(t *testing.T) { dtid string expErr string participant []*querypb.Target - }{ - { - name: "invalid dtid", - tmc: &testutil.TabletManagerClient{}, - dtid: "dtid01", - expErr: "invalid parts in dtid: dtid01", - }, { - name: "invalid transaction id", - tmc: &testutil.TabletManagerClient{}, - dtid: "ks:80-:013c", - expErr: "invalid transaction id in dtid: ks:80-:013c", - }, { - name: "only dtid", - tmc: &testutil.TabletManagerClient{}, - dtid: "testkeyspace:80-:1234", - }, { - name: "with participant", - tmc: &testutil.TabletManagerClient{}, - dtid: "testkeyspace:80-:1234", - participant: []*querypb.Target{{Keyspace: ks, Shard: "-80"}}, - }, { - name: "call error", - tmc: &testutil.TabletManagerClient{CallError: true}, - dtid: "testkeyspace:80-:1234", - participant: []*querypb.Target{{Keyspace: ks, Shard: "-80"}}, - expErr: "blocked call for ConcludeTransaction on fake TabletManagerClient", + }{{ + name: "invalid dtid", + tmc: &testutil.TabletManagerClient{}, + dtid: "dtid01", + expErr: "invalid parts in dtid: dtid01", + }, { + name: "invalid transaction id", + tmc: &testutil.TabletManagerClient{}, + dtid: "ks:80-:013c", + expErr: "invalid transaction id in dtid: ks:80-:013c", + }, { + name: "only dtid", + tmc: &testutil.TabletManagerClient{ + ReadTransactionResult: map[string]*querypb.TransactionMetadata{ + "80-": {Dtid: "bb"}, + }, }, - } + dtid: "testkeyspace:80-:1234", + }, { + name: "only dtid - empty metadata", + tmc: &testutil.TabletManagerClient{ + ReadTransactionResult: map[string]*querypb.TransactionMetadata{}, + }, + dtid: "testkeyspace:80-:1234", + }, { + name: "only dtid - fail", + tmc: &testutil.TabletManagerClient{}, + dtid: "testkeyspace:80-:1234", + expErr: "no ReadTransaction result on fake TabletManagerClient", + }, { + name: "with participant", + tmc: &testutil.TabletManagerClient{}, + dtid: "testkeyspace:80-:1234", + participant: []*querypb.Target{{Keyspace: ks, Shard: "-80"}}, + }, { + name: "call error", + tmc: &testutil.TabletManagerClient{CallError: true}, + dtid: "testkeyspace:80-:1234", + participant: []*querypb.Target{{Keyspace: ks, Shard: "-80"}}, + expErr: "blocked call for ConcludeTransaction on fake TabletManagerClient", + }} tablets := []*topodatapb.Tablet{{ Alias: &topodatapb.TabletAlias{Cell: "zone1", Uid: 100}, diff --git a/go/vt/vtctl/grpcvtctldserver/testutil/test_tmclient.go b/go/vt/vtctl/grpcvtctldserver/testutil/test_tmclient.go index f02bb23befe..76297851f14 100644 --- a/go/vt/vtctl/grpcvtctldserver/testutil/test_tmclient.go +++ b/go/vt/vtctl/grpcvtctldserver/testutil/test_tmclient.go @@ -260,6 +260,7 @@ type TabletManagerClient struct { Error error } GetUnresolvedTransactionsResults map[string][]*querypb.TransactionMetadata + ReadTransactionResult map[string]*querypb.TransactionMetadata // keyed by tablet alias. InitPrimaryDelays map[string]time.Duration // keyed by tablet alias. injects a sleep to the end of the function @@ -663,20 +664,24 @@ func (fake *TabletManagerClient) GetUnresolvedTransactions(ctx context.Context, return fake.GetUnresolvedTransactionsResults[tablet.Shard], nil } -// ConcludeTransaction is part of the tmclient.TabletManagerClient interface. -func (fake *TabletManagerClient) ConcludeTransaction(ctx context.Context, tablet *topodatapb.Tablet, dtid string, mm bool) error { +// ReadTransaction is part of the tmclient.TabletManagerClient interface. +func (fake *TabletManagerClient) ReadTransaction(ctx context.Context, tablet *topodatapb.Tablet, dtid string) (*querypb.TransactionMetadata, error) { if fake.CallError { - return fmt.Errorf("%w: blocked call for ConcludeTransaction on fake TabletManagerClient", assert.AnError) + return nil, fmt.Errorf("%w: blocked call for ReadTransaction on fake TabletManagerClient", assert.AnError) } - return nil + if fake.ReadTransactionResult == nil { + return nil, fmt.Errorf("%w: no ReadTransaction result on fake TabletManagerClient", assert.AnError) + } + + return fake.ReadTransactionResult[tablet.Shard], nil } -// ReadTransaction is part of the tmclient.TabletManagerClient interface. -func (fake *TabletManagerClient) ReadTransaction(ctx context.Context, tablet *topodatapb.Tablet, dtid string) (*querypb.TransactionMetadata, error) { +// ConcludeTransaction is part of the tmclient.TabletManagerClient interface. +func (fake *TabletManagerClient) ConcludeTransaction(ctx context.Context, tablet *topodatapb.Tablet, dtid string, mm bool) error { if fake.CallError { - return nil, fmt.Errorf("%w: blocked call for ReadTransaction on fake TabletManagerClient", assert.AnError) + return fmt.Errorf("%w: blocked call for ConcludeTransaction on fake TabletManagerClient", assert.AnError) } - return nil, nil + return nil } // FullStatus is part of the tmclient.TabletManagerClient interface.