FrankChen021 commented on issue #20312: URL: https://github.com/apache/druid/issues/20312#issuecomment-5620988699
This comment is from **OpenAI Codex**, posted at the user's request. I reviewed the relevant source at `5e71056845`, selected CI logs, and the current #20291 diff. These are the points where I would change or qualify Fable's suggestions; straightforward agreements are omitted. This was a source-and-log review, not a reproduction or validation of implemented fixes. 1. **KubernetesTaskRunnerTest: also fix the runner lifecycle; avoid making the whole client a nice mock.** A narrow cleanup `.anyTimes()` expectation is a reasonable small fix, but the test creates a local runner without stopping it. Stop every created runner in `finally`/teardown so cleanup does not outlive the test. I prefer a controllable cleanup executor, with cleanup behavior tested separately. Making the entire client a nice mock weakens unrelated verification. [Test source](https://github.com/apache/druid/blob/5e71056845b0a63fc01b09c4516f3d5fef1141a9/extensions-core/kubernetes-overlord-extensions/src/test/java/org/apache/druid/k8s/overlord/KubernetesTaskRunnerTest.java#L261-L297). 2. **ReferenceCountingResourceHolderTest: relaxing or removing the counter assertion alone is incomplete.** The GC loop exits as soon as the global leak counter changes. An unrelated cleaner can end the wait before this test's closer sets `released`. The cleaner also increments the counter before invoking the closer. Change the bounded loop to wait for **this test's `released.get()`**, then assert release and optionally retain the counter's lower-bound assertion. Apply this to both tests using the helper. [Wait loop](https://github.com/apache/druid/blob/5e71056845b0a63fc01b09c4516f3d5fef1141a9/processing/src/test/java/org/apache/druid/collections/ReferenceCountingResourceHolderTest.java#L117-L128). 3. **Separate QueryLaningTest from IngestionDockerTest.** The QueryLaning failure waits for an aggregate schema-refresh count. `LatchableEmitter` considers previously observed events since its last flush, so an event arriving before the wait is not inherently missed. Polling readiness can be appropriate, but must establish segment availability **and SQL schema readiness**, not merely rows in `sys.segments`. Inspect Broker/Historical test logs before calling this slow loading or increasing the timeout. For IngestionDockerTest, there is a concrete missing-emission path: removing the last segment removes the datasource table directly without emitting the removal metric there. A longer timeout cannot guarantee that event. **#20291 already addresses this path and adds regression coverage**, so it is relevant here as well as to TaskQueueScaleTest. I prefer that repair over only changing the test to poll. [Removal path](https://github.com/apache/druid/blob/5e71056845b0a63fc01b09c4516f3d5fef1141a9/server/src/main/java/org/apache/druid/segment/metadata/AbstractSegmentMetadataCache.java#L584-L620), [#20291](https://github.com/apache/druid/pull/20291). 4. **Router readiness: collect diagnostics before increasing the timeout.** The current timeout is already 300 seconds per pod. Capture conditions, Kubernetes events, termination reasons, and current/previous container logs, preserving the original exception if diagnostics fail. Increase the timeout only if diagnostics show healthy but slow startup; a neighboring passing commit does not establish that explanation. 5. **NVD: the API key is already configured, and routine purging defeats database caching.** The workflow supplies `NVD_API_KEY` and runs `dependency-check:purge` before every check. Remove that routine purge when introducing caching, retain/update the database, and use bounded retries. I would **not make an unsuccessful security scan silently pass**. Any cached-data fallback should have an explicit freshness policy; otherwise report that scanning could not complete. [Workflow](https://github.com/apache/druid/blob/5e71056845b0a63fc01b09c4516f3d5fef1141a9/.github/workflows/cron-job-its.yml#L51-L64). 6. **Profiler download: strengthen the non-fatal fallback.** Plain `curl --retry 3` does not retry every failure, including the reported TLS connection error. Use bounded retries covering that error (for example, `--retry-all-errors` with `--retry`), HTTP failure detection, and connection/transfer timeouts. Download to a temporary file and remove partial output on failure. Emit an empty `JFR_PROFILER_ARG_LINE` for the fallback, and send diagnostics to **stderr**, since stdout is appended directly to `$GITHUB_ENV`. [curl documentation](https://curl.se/docs/manpage.html#--retry-all-errors). 7. **Compaction: agree with bounded polling, but qualify the cause.** The helper uses `waitForNextEvent`, not a wait matching stored old events. An in-flight sync can still observe older state, and metadata synchronization is separate from segment availability. An empty SQL result does not itself establish that the datasource was absent from the Broker schema. Keep the Overlord assertion, then poll the exact expected `sys.segments` condition with a deadline and last-result diagnostics. [Helper](https://github.com/apache/druid/blob/5e71056845b0a63fc01b09c4516f3d5fef1141a9/embedded-tests/src/test/java/org/apache/druid/testing/embedded/compact/CompactionTestBase.java#L98-L103). 9. **Supervisor notice completion: use the existing completion hook.** The queue-empty race is real, but a new production API is unnecessary. `emitNoticeProcessTime` already runs after `notice.handle()`. Override it in the test supervisor to release a latch specifically for `handoff_task_group_notice`, then await that latch before the second `runInternal()`. Also control scheduled runs and stop the supervisor during cleanup. This retains the asynchronous path and establishes completion and memory visibility. [Processing order](https://github.com/apache/druid/blob/5e71056845b0a63fc01b09c4516f3d5fef1141a9/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java#L1545-L1559). Finally, “none is caused by the commit itself” is stronger than neighboring passing builds establish. They support intermittency, but do not prove independence from the change, particularly for changes affecting JVM execution. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
