IGNITE-28675 Fix flaky TxDeadlockCauseTest #testCause, #testCauseSeveralNodes

[zstan]

No description provided.

[zstan]

From documentation:
https://ignite.apache.org/docs/ignite2/latest/key-value-api/transactions
However, if a deadlock is detected, the cause of the returned TransactionTimeoutException will be TransactionDeadlockException (at least for one transaction involved in the deadlock).

[ignitetcbot]

TCBot Test Analysis

• TeamCity: --> Run :: All Results
• Branch: pull/13136/head
• Base: master
• Tested commit: 94b11ec

Possible Blockers (0)

No blockers found.

New Tests (0)

No new tests found.

[ignitetcbot]

Thanks for working on this flaky test. I have two small suggestions:

1. Please reconsider making TxDeadlockDetection.deadLockTimeout static final while TxDeadlockDetectionNoHangsTest relies on @WithSystemProperty. In the suite, earlier tests can initialize TxDeadlockDetection before the class-level property is applied, so this test may still use the default timeout. A safer option is to read IGNITE_TX_DEADLOCK_DETECTION_TIMEOUT when creating the timeout object, or keep an explicit test hook. See TxDeadlockDetection, TxDeadlockDetectionNoHangsTest, and suite order in TxDeadlockDetectionTestSuite.

2. In TxDeadlockCauseTest, the new catch stores only exceptions that already have TransactionDeadlockException. If both transaction threads fail for another reason, the test will lose the original failure and report only assertNotNull. Could we keep the first non-deadlock exception as a diagnostic fallback, but still prefer the deadlock exception when it appears? See checkCauseObject.

[zstan]

this is from our own documentation

[chesnokoff]

Let's roll back adding the final qualifier here, because it changes the semantics of TxDeadlockDetectionNoHangsTest and is not the main goal of this patch.

[chesnokoff]

and roll back renaming too

[zstan]

disagree, why do we need to support non-final definition only for test cases ?
test work properly well without this reflection changes, i run it multiple times, you can check it through TC

[chesnokoff]

My position is not really about final/non-final definition itself. My point is that it is usually safer to keep PRs as minimal as possible for a specific purpose. The main goal here is to fix TxDeadlockCauseTest, and for now it is not very clear why we also need to refactor an unrelated field and affect existing test behavior.

Green TC runs are definitely a good sign, but some flaky tests fail once in weeks or even months. That's why I'd prefer to avoid introducing additional assumptions here unless this change is really required for the fix itself.

Could you provide more reasoning on why it is safe to remove before/afterTests from TxDeadlockDetectionNoHangsTest?

[zstan]

I appent additional checks into test, plz check last commit
Also, tests restore after for hardcoded - 60000, that equal to current default but can be changed in future thus there can be a situation when test restores not a default.

[chesnokoff]

public void registerListener(@NotNull LogListener lsnr) {
        lsnr.reset();

Can we lose an already caught "Deadlock detection was timed out" message here? doTest starts grid(NODES_CNT) many times and each start calls getConfiguration(). But registerListener(LogListener) resets the listener, so a later node start can clear a timeout message that was caught before

[zstan]

I missunderstand - lsnr.check() calls after each test on: afterTest()
Can you explain how it can be a problem ? Thanks !

[chesnokoff]

I mean that lsnr.reset() can clear the listener state. Can getConfiguration() be called after Deadlock detection was timed out was already logged?

doTest() runs for about 120 seconds. During this time, the restart thread starts NODES_CNT grids many times, and each start calls getConfiguration(). The deadlock detection timeout is 60 seconds, so the timeout log can appear first. After that, another node restart can call getConfiguration() and reset the listener before afterTest() calls lsnr.check()

[zstan]

got it ! thanks, if now it`s ok i will repeatedly run a TC

[chesnokoff]

Theoretically, we still have a small race:

assertFalse(DEAD_LOCK_LSNR.check());
// catch the log
listeningLog.registerListener(DEAD_LOCK_LSNR);

Probably it would be better to use

public void registerListener(@NotNull Consumer<String> lsnr)

and make smth like

registerListener(log -> <update atomicBoolean if catch the log>)

so the atomicBoolean would be a common state for all grids

But on the other hand it makes the test more complicated. WDYT?

[zstan]

you say the proper things of cource, thanks, change a tests.

[sonarqubecloud]

Quality Gate failed

Failed conditions
3 New Code Smells (required ≤ 1)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE