Skip to content

Commit a4005e3

Browse files
nvbnvb
committed
kv: unit test PrepareTransactionForRetry and TransactionRefreshTimestamp
Informs cockroachdb#104233. This commit adds a pair of new unit tests to verify the behavior of `PrepareTransactionForRetry` and `TransactionRefreshTimestamp`. These functions will be getting more complex for cockroachdb#104233, so it will be helpful to have these tests in place. The tests also serve as good documentation. Release note: None
1 parent c5b6392 commit a4005e3

File tree

4 files changed

+257
-18
lines changed

4 files changed

+257
-18
lines changed

‎pkg/kv/kvclient/kvcoord/txn_coord_sender.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -804,9 +804,7 @@ func (tc *TxnCoordSender) UpdateStateOnRemoteRetryableErr(
804804
// not be usable afterwards (in case of TransactionAbortedError). The caller is
805805
// expected to check the ID of the resulting transaction. If the TxnCoordSender
806806
// can still be used, it will have been prepared for a new epoch.
807-
func (tc *TxnCoordSender) handleRetryableErrLocked(
808-
ctx context.Context, pErr *kvpb.Error,
809-
) *kvpb.TransactionRetryWithProtoRefreshError {
807+
func (tc *TxnCoordSender) handleRetryableErrLocked(ctx context.Context, pErr *kvpb.Error) error {
810808
// If the error is a transaction retry error, update metrics to
811809
// reflect the reason for the restart. More details about the
812810
// different error types are documented above on the metaRestart
@@ -842,7 +840,10 @@ func (tc *TxnCoordSender) handleRetryableErrLocked(
842840
tc.metrics.RestartsUnknown.Inc()
843841
}
844842
errTxnID := pErr.GetTxn().ID
845-
newTxn := kvpb.PrepareTransactionForRetry(ctx, pErr, tc.mu.userPriority, tc.clock)
843+
newTxn, assertErr := kvpb.PrepareTransactionForRetry(pErr, tc.mu.userPriority, tc.clock)
844+
if assertErr != nil {
845+
return assertErr
846+
}
846847

847848
// We'll pass a TransactionRetryWithProtoRefreshError up to the next layer.
848849
retErr := kvpb.NewTransactionRetryWithProtoRefreshError(

‎pkg/kv/kvpb/BUILD.bazel‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ go_test(
5555
srcs = [
5656
"api_test.go",
5757
"batch_test.go",
58+
"data_test.go",
5859
"errors_test.go",
5960
"node_decommissioned_error_test.go",
6061
"replica_unavailable_error_test.go",
@@ -74,6 +75,7 @@ go_test(
7475
"//pkg/util/buildutil",
7576
"//pkg/util/hlc",
7677
"//pkg/util/protoutil",
78+
"//pkg/util/timeutil",
7779
"//pkg/util/uuid",
7880
"@com_github_cockroachdb_errors//:errors",
7981
"@com_github_cockroachdb_redact//:redact",

‎pkg/kv/kvpb/data.go‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,9 @@
1212
package kvpb
1313

1414
import (
15-
"context"
16-
1715
"github.com/cockroachdb/cockroach/pkg/roachpb"
1816
"github.com/cockroachdb/cockroach/pkg/util/hlc"
19-
"github.com/cockroachdb/cockroach/pkg/util/log"
17+
"github.com/cockroachdb/errors"
2018
)
2119

2220
// PrepareTransactionForRetry returns a new Transaction to be used for retrying
@@ -34,14 +32,18 @@ import (
3432
// In case retryErr tells us that a new Transaction needs to be created,
3533
// isolation and name help initialize this new transaction.
3634
func PrepareTransactionForRetry(
37-
ctx context.Context, pErr *Error, pri roachpb.UserPriority, clock *hlc.Clock,
38-
) roachpb.Transaction {
35+
pErr *Error, pri roachpb.UserPriority, clock *hlc.Clock,
36+
) (roachpb.Transaction, error) {
37+
if pErr == nil {
38+
return roachpb.Transaction{}, errors.AssertionFailedf("nil error")
39+
}
3940
if pErr.TransactionRestart() == TransactionRestart_NONE {
40-
log.Fatalf(ctx, "invalid retryable err (%T): %s", pErr.GetDetail(), pErr)
41+
return roachpb.Transaction{}, errors.AssertionFailedf(
42+
"invalid retryable error (%T): %s", pErr.GetDetail(), pErr)
4143
}
42-
4344
if pErr.GetTxn() == nil {
44-
log.Fatalf(ctx, "missing txn for retryable error: %s", pErr)
45+
return roachpb.Transaction{}, errors.AssertionFailedf(
46+
"missing txn for retryable error: %s", pErr)
4547
}
4648

4749
txn := *pErr.GetTxn()
@@ -108,19 +110,20 @@ func PrepareTransactionForRetry(
108110
// IntentMissingErrors are not expected to be handled at this level;
109111
// We instead expect the txnPipeliner to transform them into a
110112
// TransactionRetryErrors(RETRY_ASYNC_WRITE_FAILURE) error.
111-
log.Fatalf(
112-
ctx, "unexpected intent missing error (%T); should be transformed into retry error", pErr.GetDetail(),
113-
)
113+
return roachpb.Transaction{}, errors.AssertionFailedf(
114+
"unexpected intent missing error (%T); should be transformed into retry error", pErr.GetDetail())
114115
default:
115-
log.Fatalf(ctx, "invalid retryable err (%T): %s", pErr.GetDetail(), pErr)
116+
return roachpb.Transaction{}, errors.AssertionFailedf(
117+
"invalid retryable err (%T): %s", pErr.GetDetail(), pErr)
116118
}
117119
if !aborted {
118120
if txn.Status.IsFinalized() {
119-
log.Fatalf(ctx, "transaction unexpectedly finalized in (%T): %s", pErr.GetDetail(), pErr)
121+
return roachpb.Transaction{}, errors.AssertionFailedf(
122+
"transaction unexpectedly finalized in (%T): %s", pErr.GetDetail(), pErr)
120123
}
121124
txn.Restart(pri, txn.Priority, txn.WriteTimestamp)
122125
}
123-
return txn
126+
return txn, nil
124127
}
125128

126129
// TransactionRefreshTimestamp returns whether the supplied error is a retry

‎pkg/kv/kvpb/data_test.go‎

Lines changed: 233 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,233 @@
1+
// Copyright 2023 The Cockroach Authors.
2+
//
3+
// Use of this software is governed by the Business Source License
4+
// included in the file licenses/BSL.txt.
5+
//
6+
// As of the Change Date specified in that file, in accordance with
7+
// the Business Source License, use of this software will be governed
8+
// by the Apache License, Version 2.0, included in the file
9+
// licenses/APL.txt.
10+
11+
package kvpb
12+
13+
import (
14+
"testing"
15+
16+
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation"
17+
"github.com/cockroachdb/cockroach/pkg/roachpb"
18+
"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
19+
"github.com/cockroachdb/cockroach/pkg/util/hlc"
20+
"github.com/cockroachdb/cockroach/pkg/util/timeutil"
21+
"github.com/cockroachdb/cockroach/pkg/util/uuid"
22+
"github.com/cockroachdb/errors"
23+
"github.com/stretchr/testify/require"
24+
)
25+
26+
func TestPrepareTransactionForRetry(t *testing.T) {
27+
ts1 := hlc.Timestamp{WallTime: 1}
28+
ts2 := hlc.Timestamp{WallTime: 2}
29+
tsClock := hlc.Timestamp{WallTime: 3}
30+
txn := roachpb.MakeTransaction("test", nil, isolation.Serializable, -1, ts1, 0, 99)
31+
txn2ID := uuid.MakeV4() // used if txn is aborted
32+
tests := []struct {
33+
name string
34+
err *Error
35+
expTxn roachpb.Transaction
36+
expErr bool
37+
}{
38+
{
39+
name: "no error",
40+
err: nil,
41+
expErr: true,
42+
},
43+
{
44+
name: "no txn",
45+
err: NewError(errors.New("random")),
46+
expErr: true,
47+
},
48+
{
49+
name: "random error",
50+
err: NewErrorWithTxn(errors.New("random"), &txn),
51+
expErr: true,
52+
},
53+
{
54+
name: "txn aborted error",
55+
err: NewErrorWithTxn(&TransactionAbortedError{}, &txn),
56+
expTxn: func() roachpb.Transaction {
57+
nextTxn := txn
58+
nextTxn.ID = txn2ID
59+
nextTxn.ReadTimestamp = tsClock
60+
nextTxn.WriteTimestamp = tsClock
61+
nextTxn.MinTimestamp = tsClock
62+
nextTxn.LastHeartbeat = tsClock
63+
nextTxn.GlobalUncertaintyLimit = tsClock
64+
return nextTxn
65+
}(),
66+
},
67+
{
68+
name: "read within uncertainty error",
69+
err: NewErrorWithTxn(&ReadWithinUncertaintyIntervalError{ValueTimestamp: ts2}, &txn),
70+
expTxn: func() roachpb.Transaction {
71+
nextTxn := txn
72+
nextTxn.Epoch++
73+
nextTxn.ReadTimestamp = ts2.Next()
74+
nextTxn.WriteTimestamp = ts2.Next()
75+
return nextTxn
76+
}(),
77+
},
78+
{
79+
name: "txn push error",
80+
err: NewErrorWithTxn(&TransactionPushError{
81+
PusheeTxn: roachpb.Transaction{TxnMeta: enginepb.TxnMeta{WriteTimestamp: ts2, Priority: 3}},
82+
}, &txn),
83+
expTxn: func() roachpb.Transaction {
84+
nextTxn := txn
85+
nextTxn.Epoch++
86+
nextTxn.ReadTimestamp = ts2
87+
nextTxn.WriteTimestamp = ts2
88+
nextTxn.Priority = 2
89+
return nextTxn
90+
}(),
91+
},
92+
{
93+
name: "txn retry error (reason: write too old)",
94+
err: NewErrorWithTxn(&TransactionRetryError{Reason: RETRY_WRITE_TOO_OLD}, &txn),
95+
expTxn: func() roachpb.Transaction {
96+
nextTxn := txn
97+
nextTxn.Epoch++
98+
return nextTxn
99+
}(),
100+
},
101+
{
102+
name: "txn retry error (reason: serializable)",
103+
err: NewErrorWithTxn(&TransactionRetryError{Reason: RETRY_SERIALIZABLE}, &txn),
104+
expTxn: func() roachpb.Transaction {
105+
nextTxn := txn
106+
nextTxn.Epoch++
107+
nextTxn.ReadTimestamp = tsClock
108+
nextTxn.WriteTimestamp = tsClock
109+
return nextTxn
110+
}(),
111+
},
112+
{
113+
name: "write too old error",
114+
err: NewErrorWithTxn(&WriteTooOldError{ActualTimestamp: ts2}, &txn),
115+
expTxn: func() roachpb.Transaction {
116+
nextTxn := txn
117+
nextTxn.Epoch++
118+
nextTxn.ReadTimestamp = ts2
119+
nextTxn.WriteTimestamp = ts2
120+
return nextTxn
121+
}(),
122+
},
123+
{
124+
name: "intent missing error",
125+
err: NewErrorWithTxn(&IntentMissingError{}, &txn),
126+
expErr: true,
127+
},
128+
}
129+
for _, tt := range tests {
130+
t.Run(tt.name, func(t *testing.T) {
131+
clock := hlc.NewClockForTesting(timeutil.NewManualTime(timeutil.Unix(0, tsClock.WallTime)))
132+
nextTxn, err := PrepareTransactionForRetry(tt.err, -1 /* pri */, clock)
133+
if tt.expErr {
134+
require.Error(t, err)
135+
require.True(t, errors.IsAssertionFailure(err))
136+
require.Zero(t, nextTxn)
137+
} else {
138+
require.NoError(t, err)
139+
if nextTxn.ID != txn.ID {
140+
// Eliminate randomness from ID generation.
141+
nextTxn.ID = txn2ID
142+
}
143+
require.Equal(t, tt.expTxn, nextTxn)
144+
}
145+
})
146+
}
147+
}
148+
149+
func TestTransactionRefreshTimestamp(t *testing.T) {
150+
ts1 := hlc.Timestamp{WallTime: 1}
151+
ts2 := hlc.Timestamp{WallTime: 2}
152+
txn := roachpb.MakeTransaction("test", nil, isolation.Serializable, 1, ts1, 0, 99)
153+
tests := []struct {
154+
name string
155+
err *Error
156+
expOk bool
157+
expTs hlc.Timestamp
158+
}{
159+
{
160+
name: "no error",
161+
err: nil,
162+
expOk: false,
163+
expTs: hlc.Timestamp{},
164+
},
165+
{
166+
name: "no txn",
167+
err: NewError(errors.New("random")),
168+
expOk: false,
169+
expTs: hlc.Timestamp{},
170+
},
171+
{
172+
name: "random error",
173+
err: NewErrorWithTxn(errors.New("random"), &txn),
174+
expOk: false,
175+
expTs: hlc.Timestamp{},
176+
},
177+
{
178+
name: "txn aborted error",
179+
err: NewErrorWithTxn(&TransactionAbortedError{}, &txn),
180+
expOk: false,
181+
expTs: hlc.Timestamp{},
182+
},
183+
{
184+
name: "txn retry error (reason: unknown)",
185+
err: NewErrorWithTxn(&TransactionRetryError{Reason: RETRY_REASON_UNKNOWN}, &txn),
186+
expOk: false,
187+
expTs: hlc.Timestamp{},
188+
},
189+
{
190+
name: "txn retry error (reason: write too old)",
191+
err: NewErrorWithTxn(&TransactionRetryError{Reason: RETRY_WRITE_TOO_OLD}, &txn),
192+
expOk: true,
193+
expTs: ts1,
194+
},
195+
{
196+
name: "txn retry error (reason: serializable)",
197+
err: NewErrorWithTxn(&TransactionRetryError{Reason: RETRY_SERIALIZABLE}, &txn),
198+
expOk: true,
199+
expTs: ts1,
200+
},
201+
{
202+
name: "txn retry error (reason: async write failure)",
203+
err: NewErrorWithTxn(&TransactionRetryError{Reason: RETRY_ASYNC_WRITE_FAILURE}, &txn),
204+
expOk: false,
205+
expTs: hlc.Timestamp{},
206+
},
207+
{
208+
name: "txn retry error (reason: commit deadline exceeded)",
209+
err: NewErrorWithTxn(&TransactionRetryError{Reason: RETRY_COMMIT_DEADLINE_EXCEEDED}, &txn),
210+
expOk: false,
211+
expTs: hlc.Timestamp{},
212+
},
213+
{
214+
name: "write too old error",
215+
err: NewErrorWithTxn(&WriteTooOldError{ActualTimestamp: ts2}, &txn),
216+
expOk: true,
217+
expTs: ts2,
218+
},
219+
{
220+
name: "read within uncertainty error",
221+
err: NewErrorWithTxn(&ReadWithinUncertaintyIntervalError{ValueTimestamp: ts2}, &txn),
222+
expOk: true,
223+
expTs: ts2.Next(),
224+
},
225+
}
226+
for _, tt := range tests {
227+
t.Run(tt.name, func(t *testing.T) {
228+
ok, ts := TransactionRefreshTimestamp(tt.err)
229+
require.Equal(t, tt.expOk, ok)
230+
require.Equal(t, tt.expTs, ts)
231+
})
232+
}
233+
}

0 commit comments

Comments
 (0)