-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Transaction] Fix transaction buffer client channel not active problem. #10407
[Transaction] Fix transaction buffer client channel not active problem. #10407
Conversation
cache.invalidate(topic); | ||
cb.completeExceptionally(throwable); | ||
pendingRequests.remove(requestId); |
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.
Why are you removing this line?
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.
if run here, the request don't put in pendingRequests
. so remove this line
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.
makes sense, thanks
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.
+1
cache.invalidate(topic); | ||
cb.completeExceptionally(throwable); | ||
pendingRequests.remove(requestId); |
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.
makes sense, thanks
clientCnx.registerTransactionBufferHandler(TransactionBufferHandlerImpl.this); | ||
synchronized (TransactionBufferHandlerImpl.class) { |
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.
It seems odd to use the Class object as an object monitor. Is this intentional?
Did you mean to have synchronized (TransactionBufferHandlerImpl.this) {
instead?
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.
it my mistake, good catch!
Motivation
when channel is not active, it will not invalidate the
ClientCnx
.implement
check channel active.
Verifying this change
Add the tests for it
Does this pull request potentially affect one of the following parts:
If yes was chosen, please highlight the changes
Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)