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.
| public void testStartStop() | ||
| { | ||
| Consumer<String, String> kafkaConsumer = new MockConsumer<>(OffsetResetStrategy.EARLIEST); | ||
| final MockConsumer<String, String> kafkaConsumer = new MockConsumer<>(OffsetResetStrategy.EARLIEST); |
| @Test | ||
| public void testStartWaitsForInitialEndOffsets() throws Exception | ||
| { | ||
| final MockConsumer<String, String> kafkaConsumer = new MockConsumer<>(OffsetResetStrategy.EARLIEST); |
| public void testStartStartStopStop() | ||
| { | ||
| Consumer<String, String> kafkaConsumer = new MockConsumer<>(OffsetResetStrategy.EARLIEST); | ||
| final MockConsumer<String, String> kafkaConsumer = new MockConsumer<>(OffsetResetStrategy.EARLIEST); |
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