*: refine transaction retry error messages #10466

jackysp · 2019-05-14T15:23:47Z

Signed-off-by: Shuaipeng Yu [email protected]

What problem does this PR solve?

Using [try again later] as the transaction retry marker is not suitable. It costs too many resources to search the error messages strings.Contrains(err.Error(), "[try again later]") when the error stack is very long.
Some tests, e.g. jepsen have to use the error message to determine whether the transaction is aborted as expected.
errors.Annotate sometimes only set the message in the full stack error, that means it could not be used by jepsen.

What is changed and how it works?

Do not use a string to determine whether should retry the transaction.
Add "[try again later]" for ErrWriteConflict, ErrRetryable, and ErrInfoSchemaChanged, then they will be used by some test to know the transaction is safe to retry.

Check List

Tests

Unit test

Code changes

Has exported variable/fields change

Side effects

Increased code complexity

jackysp · 2019-05-14T15:26:07Z

domain/domain.go

@@ -1051,7 +1051,7 @@ var (
 	// ErrInfoSchemaExpired returns the error that information schema is out of date.
 	ErrInfoSchemaExpired = terror.ClassDomain.New(codeInfoSchemaExpired, "Information schema is out of date.")
 	// ErrInfoSchemaChanged returns the error that information schema is changed.
-	ErrInfoSchemaChanged = terror.ClassDomain.New(codeInfoSchemaChanged, "Information schema is changed.")
+	ErrInfoSchemaChanged = terror.ClassDomain.New(codeInfoSchemaChanged, "Information schema is changed."+kv.TxnRetryableMark)


There are three types of errors could be safe to retry the transaction,

ErrInfoSchemaChanged

ErrWriteConflict

ErrRetryable

add the marker for them, to make jepsen happy.

jackysp · 2019-05-14T15:26:57Z

kv/error.go

 var (
 	// ErrClosed is used when close an already closed txn.
 	ErrClosed = terror.ClassKV.New(codeClosed, "Error: Transaction already closed")
 	// ErrNotExist is used when try to get an entry with an unexist key from KV store.
 	ErrNotExist = terror.ClassKV.New(codeNotExist, "Error: key not exist")
-	// ErrConditionNotMatch is used when condition is not met.


some errors and code are removed, because they are no used.

jackysp · 2019-05-14T15:27:32Z

kv/error.go

 	// ErrKeyExists returns when key is already exist.
 	ErrKeyExists = terror.ClassKV.New(codeKeyExists, "key already exist")
 	// ErrNotImplemented returns when a function is not implemented yet.
 	ErrNotImplemented = terror.ClassKV.New(codeNotImplemented, "not implemented")
+	// ErrWriteConflict is the error when the commit meets an write conflict error.
+	ErrWriteConflict = terror.ClassKV.New(mysql.ErrWriteConflict, mysql.MySQLErrName[mysql.ErrWriteConflict]+TxnRetryableMark)


Move the ErrWriteConflict here to resolve the import circle.

jackysp · 2019-05-14T15:28:18Z

kv/error.go

-		ErrConditionNotMatch.Equal(err) ||
-		// TiKV exception message will tell you if you should retry or not
-		strings.Contains(err.Error(), "try again later") {
+	if ErrRetryable.Equal(err) || ErrWriteConflict.Equal(err) {


handle ErrRetryable and ErrWriteConflict here

rename the function to IsTxnRetryableError is better

jackysp · 2019-05-14T15:29:05Z

store/tikv/2pc.go

@@ -862,17 +862,6 @@ func (c *twoPhaseCommitter) execute(ctx context.Context) error {
 		return errors.Trace(err)
 	}

-	// mockGetTSErrorInRetry should wait MockCommitErrorOnce first, then will run into retry() logic.


PD timeout should be handled in backoffs.

jackysp · 2019-05-14T15:30:15Z

store/tikv/snapshot.go

@@ -311,14 +312,18 @@ func extractLockFromKeyErr(keyErr *pb.KeyError) (*Lock, error) {
 }

 func extractKeyErr(keyErr *pb.KeyError) error {
+	failpoint.Inject("ErrMockRetryableOnly", func(val failpoint.Value) {


Add this to mock conflict == nil, but retryable != nil, e.g. error: txnLockNotFound.

jackysp · 2019-05-14T15:30:55Z

store/tikv/snapshot.go

 	if keyErr.Conflict != nil {
-		err := newWriteConflictError(keyErr.Conflict)
-		return errors.Annotate(err, txnRetryableMark)
+		return newWriteConflictError(keyErr.Conflict)


txnRetryableMark -> ErrWriteConflict
errors.Annotate will make the txnRetryableMark at the end of the error stack.

jackysp · 2019-05-14T15:31:11Z

store/tikv/snapshot.go

-		err := errors.Errorf("tikv restarts txn: %s", keyErr.GetRetryable())
-		logutil.Logger(context.Background()).Debug("error", zap.Error(err))
-		return errors.Annotate(err, txnRetryableMark)
+		return kv.ErrRetryable.GenWithStackByArgs("tikv restarts txn: " + keyErr.GetRetryable())


txnRetryableMark -> ErrRetryable

jackysp · 2019-05-14T15:31:46Z

store/tikv/txn.go

@@ -305,7 +305,7 @@ func (txn *tikvTxn) Commit(ctx context.Context) error {
 	defer txn.store.txnLatches.UnLock(lock)
 	if lock.IsStale() {
 		err = errors.Errorf("txnStartTS %d is stale", txn.startTS)
-		return errors.Annotate(err, txnRetryableMark)
+		return kv.ErrWriteConflict.GenWithStackByArgs(txn.startTS, 0, "is stale")


txnRetryableMark -> ErrWriteConflict

codecov · 2019-05-14T15:41:39Z

Codecov Report

Merging #10466 into master will increase coverage by 0.0039%.
The diff coverage is 87.0967%.

@@               Coverage Diff                @@
##             master     #10466        +/-   ##
================================================
+ Coverage   77.3044%   77.3084%   +0.0039%     
================================================
  Files           412        412                
  Lines         86722      86715         -7     
================================================
- Hits          67040      67038         -2     
+ Misses        14525      14521         -4     
+ Partials       5157       5156         -1

jackysp · 2019-05-14T15:42:03Z

/run-all-tests

jackysp · 2019-05-14T15:45:51Z

/run-integration-common-test

coocood · 2019-05-15T02:51:19Z

kv/error.go

-	// errors which SQL layer can safely retry.
-	ErrRetryable = terror.ClassKV.New(codeRetryable, "Error: KV error safe to retry")
+	// ErrRetryable is used when KV store occurs RPC error or some other errors which SQL layer can safely retry.
+	ErrRetryable = terror.ClassKV.New(codeRetryable, "Error: KV error safe to retry %s"+TxnRetryableMark)


Rename to ErrTxnRetryable is better.

coocood · 2019-05-15T03:02:20Z

kv/error.go

 )

+// TxnRetryableMark is used to uniform the commit error messages which could retry the transaction.
+const TxnRetryableMark = " [try again later]"


Can we remove the leading space?

coocood · 2019-05-15T03:11:48Z

kv/error.go

-	// ErrRetryable is used when KV store occurs RPC error or some other
-	// errors which SQL layer can safely retry.
-	ErrRetryable = terror.ClassKV.New(codeRetryable, "Error: KV error safe to retry")
+	// ErrRetryable is used when KV store occurs RPC error or some other errors which SQL layer can safely retry.


Better add comment
currently, the err is returned when lock not found in Commit, subjective to change in the future.

coocood · 2019-05-15T03:53:41Z

kv/error.go

+	// ErrTxnRetryable is used when KV store occurs retryable error which SQL layer can safely retry the transaction.
+	// When using TiKV as the storage node, the error is returned ONLY when lock not found (txnLockNotFound) in Commit,
+	// subject to change it in the future.
+	ErrTxnRetryable = terror.ClassKV.New(codeRetryable, "Error: KV error safe to retry %s "+TxnRetryableMark)


codeRetryable -> 'codeTxnRetryable`

addressed, PTAL

coocood · 2019-05-15T03:55:04Z

session/session.go

@@ -543,9 +543,9 @@ func (s *session) isInternal() bool {

 func (s *session) isRetryableError(err error) bool {


isTxnRetryableError

addressed, PTAL

coocood · 2019-05-15T05:08:51Z

LGTM

lysu · 2019-05-15T05:29:25Z

store/tikv/snapshot.go

 	prettyWriteKey(&buf, conflict.Key)
 	buf.WriteString(" primary=")
 	prettyWriteKey(&buf, conflict.Primary)
-	return ErrWriteConflict.FastGen(buf.String())


Maybe we should enhance FastGen by use pingcap/errors#13 :D

for high conflict situation, control logic error should never generate stack.

Signed-off-by: Shuaipeng Yu <[email protected]>

jackysp · 2019-05-15T09:46:40Z

/run-all-tests

lysu

LGTM

jackysp added the type/enhancement The issue or PR belongs to an enhancement. label May 14, 2019

jackysp requested review from coocood and lysu May 14, 2019 15:23

jackysp commented May 14, 2019

View reviewed changes

jackysp requested a review from cwen0 May 14, 2019 15:42

jackysp added the priority/P1 The issue has P1 priority. label May 14, 2019

jackysp added the status/all tests passed label May 14, 2019

coocood reviewed May 15, 2019

View reviewed changes

coocood added the status/LGT1 Indicates that a PR has LGTM 1. label May 15, 2019

lysu reviewed May 15, 2019

View reviewed changes

lysu mentioned this pull request May 15, 2019

terror: enhance terror FastGen to gen nostack error pingcap/parser#326

Merged

jackysp added 3 commits May 15, 2019 17:36

refine transaction retry error messages

3dcc93a

Signed-off-by: Shuaipeng Yu <[email protected]>

fix CI

857c321

Signed-off-by: Shuaipeng Yu <[email protected]>

address comments

6114b96

Signed-off-by: Shuaipeng Yu <[email protected]>

jackysp added 3 commits May 15, 2019 17:36

address comment

5ae421d

Signed-off-by: Shuaipeng Yu <[email protected]>

address comment

82ea513

Signed-off-by: Shuaipeng Yu <[email protected]>

address comments

b8c65fc

Signed-off-by: Shuaipeng Yu <[email protected]>

jackysp force-pushed the refine_errmsg branch from 5f1ace6 to b8c65fc Compare May 15, 2019 09:38

lysu reviewed May 15, 2019

View reviewed changes

lysu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 15, 2019

lysu approved these changes May 15, 2019

View reviewed changes

jackysp merged commit d97e7d9 into pingcap:master May 15, 2019

jackysp deleted the refine_errmsg branch May 29, 2019 07:35

alivxxx mentioned this pull request Oct 22, 2019

store/tikv: add error code for write conflict #12878

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: refine transaction retry error messages #10466

*: refine transaction retry error messages #10466

jackysp commented May 14, 2019 •

edited

Loading

jackysp May 14, 2019 •

edited

Loading

jackysp May 14, 2019

jackysp May 14, 2019

jackysp May 14, 2019

coocood May 15, 2019

jackysp May 14, 2019

jackysp May 14, 2019

jackysp May 14, 2019 •

edited

Loading

jackysp May 14, 2019

jackysp May 14, 2019

codecov bot commented May 14, 2019 •

edited

Loading

jackysp commented May 14, 2019

jackysp commented May 14, 2019

coocood May 15, 2019 •

edited

Loading

coocood May 15, 2019

coocood May 15, 2019

coocood May 15, 2019

jackysp May 15, 2019

coocood May 15, 2019

jackysp May 15, 2019

coocood commented May 15, 2019

lysu May 15, 2019

lysu May 15, 2019

jackysp commented May 15, 2019

lysu left a comment

		@@ -543,9 +543,9 @@ func (s *session) isInternal() bool {

		func (s *session) isRetryableError(err error) bool {

*: refine transaction retry error messages #10466

*: refine transaction retry error messages #10466

Conversation

jackysp commented May 14, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

jackysp May 14, 2019 • 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

jackysp May 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 14, 2019 • edited Loading

Codecov Report

jackysp commented May 14, 2019

jackysp commented May 14, 2019

coocood May 15, 2019 • 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

coocood commented May 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackysp commented May 15, 2019

lysu left a comment

Choose a reason for hiding this comment

jackysp commented May 14, 2019 •

edited

Loading

jackysp May 14, 2019 •

edited

Loading

jackysp May 14, 2019 •

edited

Loading

codecov bot commented May 14, 2019 •

edited

Loading

coocood May 15, 2019 •

edited

Loading