ignitetcbot commented on PR #13494: URL: https://github.com/apache/ignite/pull/13494#issuecomment-5357134434
AI-assisted re-review of the updated implementation (`ac683a56651b`): the previous blocking concerns have been addressed. The executor is now node-owned through `PoolProcessor`, is shut down with the node, allows its core threads to time out, is exposed through executor metrics, and no longer relies on unsafe double-checked locking or a process-wide production singleton. I did not find any new functional blockers in the current diff. A few non-blocking suggestions remain: 1. Add coverage for `IGNITE_IDLE_VERIFY_POOL_SIZE`, including applying a custom value and rejecting zero/negative values. 2. Add a lifecycle test confirming that idle threads expire and the executor is shut down when the node stops. 3. Reset the test-only `EXECUTOR_SERVICE` override in a `finally` block, so it cannot leak into subsequent tests if `super.afterTest()` fails. 4. Documentation grammar: `These pool threads expires` should be `These pool threads expire`. It may also be useful to document the actual current idle timeout (60 seconds). Overall, this version is architecturally much safer than the earlier iterations and looks ready for the relevant core/control-utility test suites. -- 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]
