[close #776] Fix prewrite & commit log info to debug(#776) #777
Conversation
| @@ -181,7 +180,6 @@ public ClientRPCResult commit( | |||
| // TODO: check this logic to see are we satisfied? | |||
| private boolean retryableException(Exception e) { | |||
| return e instanceof TiClientInternalException | |||
| || e instanceof KeyException | |||
There was a problem hiding this comment.
Delete this will be a break change. Some KeyException also need the retry, why do you want to exclude this?
If you still want to exclude some KeyException, please judge the type. here is an example
if (e instanceof KeyException) {
Kvrpcpb.KeyError ke = ((KeyException) e).getKeyError();
if (ke == null) return true;
if (!ke.getAbort().isEmpty()
|| ke.hasConflict()
|| ke.hasAlreadyExist()
|| ke.hasDeadlock()
|| ke.hasCommitTsExpired()
|| ke.hasTxnNotFound()) return false;
return true;
}
There was a problem hiding this comment.
ok，I review it again
| @@ -76,4 +99,170 @@ public void autoClosableTest() throws Exception { | |||
| executorService)) {} | |||
| Assert.assertTrue(executorService.isShutdown()); | |||
| } | |||
|
|
|||
| @Test | |||
| public void prewriteWriteConflictFastFailTest() throws Exception { | |||
There was a problem hiding this comment.
It seems this test has nothing to do with this pr, right?
There was a problem hiding this comment.
This test is based on the new constructor method added in this PR #775.
Do you mind move it to that PR, or the test will fail
There was a problem hiding this comment.
this test for verify write conflict will aways retry ,but need fast fail
There was a problem hiding this comment.
could you explain why this test was added? Or what scenes/feature this test for
shiyuhang0
changed the title
shiyuhang0
changed the title
shiyuhang0
changed the title
shiyuhang0
changed the title
| @@ -31,7 +31,7 @@ public KeyException(String errMsg) { | |||
| } | |||
|
|
|||
| public KeyException(Kvrpcpb.KeyError keyErr) { | |||
| super("Key exception occurred"); | |||
| super("Key exception occurred " + keyErr.toString()); | |||
There was a problem hiding this comment.
Why do you need to print error msg here?
Have you tried getKeyErr()? it should return the error and you should be able to get the error msg from getKeyErr().toString()
|
@jackjeyis thanks for your contribution. Is there any reason to change the log info? Does it cause any problem now? |
What problem does this PR solve?
Issue Number: close #issue_number
Problem Description: TBD
What is changed and how does it work?
Code changes
Check List for Tests
This PR has been tested by at least one of the following methods:
Side effects
Related changes