-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 transactionMetadata store recover timeout problem. #10162
[Transaction] Fix transactionMetadata store recover timeout problem. #10162
Conversation
/pulsarbot run-failure-checks |
@@ -40,11 +40,10 @@ | |||
private final TripleLongPriorityQueue priorityQueue = new TripleLongPriorityQueue(); | |||
private final long tickTimeMillis; | |||
private final Clock clock; | |||
private Timeout currentTimeout; | |||
private volatile Timeout currentTimeout; |
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.
Do we need volatile
here? Seem it is protected by the synchronized
.
…r_problem # Conflicts: # pulsar-transaction/coordinator/src/main/java/org/apache/pulsar/transaction/coordinator/impl/MLTransactionMetadataStore.java
This PR introduced a flaky test #10310 . Please fix. |
field.setAccessible(true); | ||
ConcurrentMap<TxnID, Pair<TxnMeta, List<Position>>> txnMap = | ||
(ConcurrentMap<TxnID, Pair<TxnMeta, List<Position>>>) field.get(transactionMetadataStore); | ||
assertEquals(txnMap.size(), 1); |
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.
@congbobo184 This assertion assertEquals(txnMap.size(), 1);
is flaky (#10310), could it be removed?
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 for removing it.
@lhotari can you please send a PR ?
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.
@eolivelli Done, #10313
Motivation
currentTime
+transactionTimeout
, it is not right. it need to usestartTransactionTime
+transactionTimeout
.priorityQueue
implement
fix the logical of the transaction timeout.
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)