rezataroosheh commented on PR #13639: URL: https://github.com/apache/ignite/pull/13639#issuecomment-6642468471
@zstan, I am eager to hear from you about a recap of my findings: The static initializer of `GridAbstractTest` is responsible for starting the test clock. Assume we have two tests: A doesn't extend `GridAbstractTest`, and B extends `GridAbstractTest`. 1. If B starts first, we never see this issue. 2. If A starts first but doesn't leak a node, and after that B starts, we never see this issue in other tests, regardless of whether they extend `GridAbstractTest`. 3. If A starts first and leaks a node, and B starts while that node is still running, the static initializer sees `gridCnt != 0` and doesn't start the test clock. **The static initializer runs only once, so in this scenario the test clock will never start.** Only the leaked node's `ignite-clock` is running. When that node is stopped later, `gridCnt` drops to 0 and `ignite-clock` stops, so any test that doesn't start a node sees a frozen clock. This PR changes the static initializer, so it always starts the test clock and takes its `gridCnt` slot, so `gridCnt` never drops to 0 when a leaked node stops. Another option would be to make the static initializer run before any test, but most modules use JUnit 4, which doesn't have `LauncherSessionListener`, and I couldn't find a straightforward way that also works when tests run in IDEs or pipelines. Please correct me if I am wrong. To prove scenarios 1 and 3, I used your patch and changed the test order. On master (7b4a2a91fd): | Order | `gridCnt` assert | `testLongPlanningTimeout` | | --- | --- | --- | | Scenario 3: `ScriptTestSuite` (leaks 2 nodes), `IndexSearchBoundsPlannerTest` (its `@AfterAll` stops them), `PlannerTimeoutTest` | Commented out, as in your run | Fails: "Exception has not been thrown" | | Scenario 1: `IndexSearchBoundsPlannerTest` (starts the test clock), `ScriptTestSuite` (leaks 2 nodes), `IndexSearchBoundsPlannerTest` (stops them), `PlannerTimeoutTest` | Kept | Passes | The failure comes from the frozen clock: time doesn't move, so the 1 s planner timeout never fires. Your repro covers scenario 3 up to the leak. `PlannerTimeoutTest` passes there because the leaked nodes are still running, so the order change is needed to stop them first. With this PR (fe2d92021b), the scenario 3 order passes, with its assert kept. To reproduce it, on top of your patch: - For scenario 3, set `@Order(1)` on `IndexSearchBoundsPlannerTest` and `@Order(2)` on `PlannerTimeoutTest`, comment out the `gridCnt` assert in `PlannerTimeoutTest`, then run `IgniteCalciteTestSuite2`. - For scenario 1, keep the assert and also add `IndexSearchBoundsPlannerTest.class` at the top of `@SelectClasses` in `IgniteCalciteTestSuite2`. Note: The other failures are the same across all runs: the 25 `TpchQueryPlannerTest` tests and the `ScriptTestSuite` LIMIT test. Could you please try it when you have time and let me know whether this fix can go ahead or what you would like changed? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
