-
Notifications
You must be signed in to change notification settings - Fork 8.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Success state after retrying is synchronized back to the client, and print log. #861
Success state after retrying is synchronized back to the client, and print log. #861
Conversation
merge for name change
Codecov Report
@@ Coverage Diff @@
## develop #861 +/- ##
=============================================
- Coverage 38.37% 38.26% -0.11%
Complexity 1046 1046
=============================================
Files 218 218
Lines 8670 8694 +24
Branches 1080 1083 +3
=============================================
Hits 3327 3327
- Misses 4923 4947 +24
Partials 420 420
Continue to review full report at Codecov.
|
tm/src/test/java/io/seata/tm/api/DefaultFailureHandlerImplTest.java
Outdated
Show resolved
Hide resolved
tm/src/test/java/io/seata/tm/api/DefaultFailureHandlerImplTest.java
Outdated
Show resolved
Hide resolved
The retry number is unlimited ? It's better to add a limit for retry number. |
Good advice, I will modify it later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments
|
||
private static final int TICKS_PER_WHEEL = 8; | ||
|
||
private HashedWheelTimer timer = new HashedWheelTimer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need custom threadFactory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we should.
return; | ||
} | ||
isStopped = shouldStop(tx, required); | ||
timer.newTimeout(this, SCHEDULE_INTERVAL_SECONDS, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use exponential backoff 10s,20s,40s.... ns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not necessary in such a scenario.
private boolean shouldStop(final GlobalTransaction tx, GlobalStatus required){ | ||
try { | ||
GlobalStatus status = tx.getStatus(); | ||
LOGGER.warn("transaction[" + tx.getXid() + "] current status is [" + status + "]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will modify it later.
LOGGER.error("transaction[" + tx.getXid() + "] retry fetch status times exceed the limit [" + RETRY_MAX_TIMES + " times]"); | ||
return; | ||
} | ||
isStopped = shouldStop(tx, required); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments
LOGGER.error("transaction[" + tx.getXid() + "] retry fetch status times exceed the limit [" + RETRY_MAX_TIMES + " times]"); | ||
return; | ||
} | ||
isStopped = shouldStop(tx, required); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…print log. (apache#861) * Success state after retrying is synchronized back to the client, and print log.
Ⅰ. Describe what this PR did
At present, the client can only perceive the first commit or rollback failure. In fact, the transaction will be retried on the server at this time, so the client needs to be aware of the final state of the retry transaction.
At first I thought I needed to design the server push mode, and found that need to change a lot of code.
Eventually I used the client polling mode and added my logic to DefaultFailureHandlerImpl.
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews