ignitetcbot commented on PR #13497: URL: https://github.com/apache/ignite/pull/13497#issuecomment-5346825736
AI-assisted code review found three concerns in this extended executor implementation: 1. **[P1] Tasks can be rejected while reusable workers are idle.** The custom queue returns `false` whenever `getActiveCount() < getMaximumPoolSize()` to force `ThreadPoolExecutor` to create another worker. However, `ThreadPoolExecutor` limits worker creation using the total worker count, not the active count. After the pool has reached its maximum size, some workers may become idle but remain alive for 20 seconds. A new submission then sees `activeCount < maximumPoolSize`, `offer()` returns `false`, and `addWorker()` fails because the total worker count is already at the maximum; `ThreadPoolExecutor` consequently rejects the task instead of queuing it. A repeated `idle_verify` during the keep-alive interval can therefore fail with `RejectedExecutionException`. 2. **[P2] Unsafe publication remains.** `initOrGetVerifyExecutor` uses double-checked locking, but `EXECUTOR_SERVICE` is not `volatile`. The unsynchronized outer read does not guarantee safe publication under the Java Memory Model. Please make the field `volatile` or synchronize every access, including the test-only setter. 3. **Executor lifecycle is only partially addressed.** Allowing idle workers to expire avoids permanently keeping non-daemon threads alive, but the static executor is still not owned by an Ignite node and is not shut down when that node stops. Active or queued verification tasks can therefore outlive the node, and the singleton is reused across node restarts or multiple nodes in the same JVM. A node-owned lifecycle-managed executor would avoid these cases without relying on custom queue behavior. -- 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]
