[incubator-kie-drools-5847] Flaky KieContainerTest.testIncrementalCom… #6002
Conversation
…pilationSynchronization timeout
| while (!previousRuleExists(kieContainer, i)) { // if rule1 exists, we can change it to rule2 | ||
| try { | ||
| Thread.sleep(1); | ||
| } catch (InterruptedException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
See the analysis: #5848 (comment)
The kbase update modification will be done by its own thread immediately or by main thread polling kbaseModificationsQueue later.
https://github.com/apache/incubator-kie-drools/blob/main/drools-kiesession/src/main/java/org/drools/kiesession/rulebase/SessionsAwareKnowledgeBase.java#L287-L297
public void enqueueModification(Runnable modification) {
if ( tryLockAndDeactivate() ) {
try {
modification.run();
} finally {
unlockAndActivate();
}
} else {
kbaseModificationsQueue.offer(modification);
}
}So we cannot assume that the update modifications will be done in the expected order. In the failure case, remove rule1, add rule2 was executed before remove rule0, add rule1, so there was no chance to remove rule1.
I think it's not an engine bug, but we should fix the test case. With the PR fix, we make sure that we call updateToVersion only when the previous rule exists.
The test case can still test the multi-thread concurrency of fireAllRules and updateToVersion. Is it fine for the test purpose? @mariofusco
Btw, this PR while and Thread.sleep logic is a naive approach and can be re-written by ScheduledExecutorService, but I think this one is concise and easier to understand.
There was a problem hiding this comment.
@tkobayas Great work in analyzing this issue and yes I agree that the problem is in how the test is written and its wrong assumption on the strict execution order and not in the actual code.
|
@mariofusco @lucamolteni @pibizza , please review. Thanks! |
…pilationSynchronization timeout (apache#6002)
…pilationSynchronization timeout
Issue: