Fix flaky SharedTsBlockQueue concurrency test - #18244
Conversation
JackieTien97
left a comment
There was a problem hiding this comment.
One non-blocking suggestion to improve failure diagnostics.
| return null; | ||
| }); | ||
|
|
||
| sender.get(30, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Could we wait for whichever worker completes first here? If the receiver fails before the sender finishes, it stops freeing queue memory; the sender can then block once the pool fills, so this Future.get call reports a 30-second TimeoutException and receiver.get is never reached. That hides the receiver's actual exception, even though this change aims to propagate worker failures. Using an ExecutorCompletionService with a shared deadline (and cancelling the sibling task on failure) would make this fail fast and preserve the root cause.
There was a problem hiding this comment.
Applied in af46d3d. The test now consumes worker results through ExecutorCompletionService with one shared 30-second deadline, so the first completed worker is observed immediately. If either worker fails or the shared deadline expires, both futures are cancelled before rethrowing, preserving the root cause. SharedTsBlockQueueTest passes (2 tests).
Description
SharedTsBlockQueueTest.concurrencyTestused a fixed JUnit timeout while polling counters, printing progress, and recursively scheduling listener tasks. Under CI load, the test could time out without surfacing worker failures, and its executor was never shut down.This change makes the test deterministic by:
Future.getTesting
mvn -o -nsu -Ddevelocity.off=true spotless:check -pl iotdb-core/datanodeSharedTsBlockQueueTestdirectly: 2 tests passedA full reactor test attempt was blocked before reaching DataNode by an existing generated Thrift compilation error:
TaskType.javacould not resolvejavax.annotation.