-
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
[fix][test] fix testEndTxnWhenCommittingOrAborting flaky test #18318
[fix][test] fix testEndTxnWhenCommittingOrAborting flaky test #18318
Conversation
if (this.interceptor != null) { | ||
this.interceptor.txnEnded(txnID.toString(), TxnAction.ABORT_VALUE); | ||
} |
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 do we need to delete this part?
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 end transaction throws an exception, it doesn't mean the transaction has been ended, so we don't need to record it as aborted
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.
Maybe the title should no longer be “fix test“?
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.
Agree with @poorbarcode. Is this change related to this flaky test? If not, a new PR to delete it may be better.
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.
Good work!
if (this.interceptor != null) { | ||
this.interceptor.txnEnded(txnID.toString(), TxnAction.ABORT_VALUE); | ||
} |
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.
Maybe the title should no longer be “fix test“?
@poorbarcode mainly to fix the flaky test, this is just to remove the wrong logic |
The pr had no activity for 30 days, mark with Stale label. |
new flaky-test case: |
…nCommittingOrAborting # Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PulsarCommandSenderImpl.java
Fixes: #18318
issue: https://github.com/apache/pulsar/actions/runs/3383985489/jobs/5620564078
Motivation
Transaction op count update after the write response, so we need to wait for the change to succeed
https://github.com/congbobo184/pulsar/blob/2b441bd2ab4b71d5bc182f85c62fbea633c4ce4b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PulsarCommandSenderImpl.java#L314-L317
https://github.com/congbobo184/pulsar/blob/2b441bd2ab4b71d5bc182f85c62fbea633c4ce4b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PulsarCommandSenderImpl.java#L334-L337
Modifications
wait count changed
Verifying this change
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository:
congbobo184#7