fix: Wait for Kafka lookup startup catch-up - #19858
Conversation
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 2 of 2 changed files. Found one startup-timeout cleanup race where the Kafka worker may keep writing after its cache is disposed.
This is an automated review by Codex GPT-5.6-Sol
| doubleEventCount.incrementAndGet(); | ||
| LOG.trace("Placed key[%s] val[%s]", key, message); | ||
| } | ||
| if (startingReads.getCount() > 0 |
There was a problem hiding this comment.
[P2] Wait for the catch-up worker before disposing its cache
When a finite connectTimeout expires while this new catch-up loop is processing a backlog, start() calls future.cancel(true) and immediately cacheHandler.close(). Cancellation only interrupts the worker; the record loop has no interrupt check, so it can continue put/remove calls after close. With the off-heap cache manager, close deletes the underlying MapDB map, racing those writes against deletion. Coordinate worker termination, or move cache disposal into the worker's completion path, before closing the cache.
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
Reviewed 2 of 2 changed files. The prior cache-disposal race is resolved, but the replacement cleanup path can block process startup indefinitely after the configured timeout.
This is an automated review by Codex GPT-5.6-Sol
| boolean interrupted = false; | ||
| while (!executorService.isTerminated()) { | ||
| try { | ||
| executorService.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS); |
There was a problem hiding this comment.
[P1] Do not wait forever after connectTimeout expires
After start() reaches its configured timeout, this call waits indefinitely for executor termination. future.cancel(true) only requests interruption and cannot guarantee that consumer code exits; the updated test demonstrates the regression by configuring a 1 ms timeout yet requiring start() to remain blocked until an uninterruptible poll is manually released. LookupReferencesManager.startLookups() waits for every lookup result, so one stuck Kafka consumer can now block lookup-manager and process startup indefinitely instead of returning false after connectTimeout. Preserve cache safety without synchronously waiting forever, for example by transferring cache disposal to worker completion while allowing start() to return after bounded cancellation.
|
@FrankChen021
|
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the updated head for correctness, edge cases, concurrency, and integration risks; no issues found.
Reviewed 2 of 2 changed files. The prior unbounded-startup-wait finding is resolved while cache disposal remains ordered after the Kafka worker exits.
This is an automated review by Codex GPT-5.6-Sol
Fixes #19575
Description
KafkaLookupExtractorFactorypreviously reported a lookup as started immediately after the consumer's firstpoll(), even when the consumer had not finished reading the existing topic data. This allowed newly started brokers to serve queries using a partially populated lookup.This change:
The existing
connectTimeoutcontinues to bound how longstart()waits. A value of0retains the existing do-not-wait behavior.A regression test verifies that
start()remains blocked while the consumer is behind and succeeds after all initial records have been applied to the lookup.Testing
mvn test -pl extensions-core/kafka-extraction-namespace -am \ -Dtest=org.apache.druid.query.lookup.KafkaLookupExtractorFactoryTest \ -Dsurefire.failIfNoSpecifiedTests=false \ -Dweb.console.skip=true \ -T1C