[Transaction] Fix transaction timeout tracker expired problem. #10366
Conversation
| @Test | ||
| public void testTimeoutTrackerExpired() throws Exception { | ||
| pulsar.getTransactionMetadataStoreService().addTransactionMetadataStore(TransactionCoordinatorID.get(0)); | ||
| Awaitility.await().atMost(2000, TimeUnit.MILLISECONDS) |
There was a problem hiding this comment.
Please remove 'atMost' as @lhotari recently suggested
| txnMap.forEach((txnID, txnMetaListPair) -> | ||
| Assert.assertEquals(txnMetaListPair.getLeft().status(), TxnStatus.OPEN)); | ||
| Awaitility.await().atLeast(1000, TimeUnit.MICROSECONDS).atMost(3000, TimeUnit.MILLISECONDS) | ||
| .until(() -> txnMap.size() == 0); |
There was a problem hiding this comment.
This kind of check is probably going to become a new flaky test, because we want that something happens exactly in a given time range, but in case of long GC pause (or other unpredictable slowdown) we are going to fail the test.
Is there another way to implement this check?
There was a problem hiding this comment.
Regarding the timeout check, I think it can only be achieved by waiting. any good idea do you have?
| field.setAccessible(true); | ||
| ConcurrentMap<TxnID, Pair<TxnMeta, List<Position>>> txnMap = | ||
| (ConcurrentMap<TxnID, Pair<TxnMeta, List<Position>>>) field.get(transactionMetadataStore); | ||
|
|
There was a problem hiding this comment.
Don't we use Powermock Whitebox.getInternalState?
There was a problem hiding this comment.
We have not exposed the interface to call all transaction metadata information.
There was a problem hiding this comment.
I meant to use Whitebox.getInternalState that is a useful utility in order to not using directly reflection, that it is something that breaks from one JDK version to the next one.
not a problem for me, just a suggestion
| field.setAccessible(true); | ||
| ConcurrentMap<TxnID, Pair<TxnMeta, List<Position>>> txnMap = | ||
| (ConcurrentMap<TxnID, Pair<TxnMeta, List<Position>>>) field.get(transactionMetadataStore); | ||
|
|
There was a problem hiding this comment.
I meant to use Whitebox.getInternalState that is a useful utility in order to not using directly reflection, that it is something that breaks from one JDK version to the next one.
not a problem for me, just a suggestion
Motivation
now transaction timeout tracker in expired state, it will not cancel successfully. so should add the timeout expired check
implement
add the timeout expired check
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)