Repository navigation
test: Make EmbeddedKafkaSupervisorTest wait for Broker queryability - #20620
FrankChen021 merged 2 commits into
Conversation
…edPartitions waitForSelectedRows only waited for the indexer's ingest/events/processed metric, so the SQL assertions could run before the Broker had the datasource in its schema and fail with "Object 'datasource_xxx' not found". Wait for ingest/handoff/count instead, then for the segments to be queryable through the Broker with waitForAllSegmentsToBeQueryable. Handoff follows publish, which commits the partition 1 offset before the supervisor spec is replaced, so the replacement supervisor resumes from it. Submit both specs through submitSupervisor so the shared teardown suspends the supervisor and cancels its tasks even when the test fails.
… SQL checks test_runKafkaSupervisor checked the row count after an indexer-only ingest/events/processed wait, and header filtering queried right after handoff without checking the Broker. Add EmbeddedClusterApis.waitForSegmentsToBeQueryable, which takes the segment count so it also covers unpublished realtime segments, and make waitForAllSegmentsToBeQueryable delegate to it. test_runKafkaSupervisor waits for its 10 segments with it. Header filtering and the selected-partitions test share a waitForHandedOffRows helper.
|
The daily flaky test triage does not report the failure of this test, could you show me a CI job link so that I can investigate why it was not reported? |
|
Thanks for checking this out Frank. I can't share the failure with you here because it happened in CI on our private downstream fork. The test file is the same as upstream though. Here is the relevant part of the log; all 4 surefire attempts failed at the same line: Claude was also able to reproduce it on apache master: with |
|
Noted and thanks for the fix. just curious that the test didn't fail for commits on master branch and other PRs after that one was merged. Tests of ingestion are very like flaky based on the triage report, I haven't come up with ideas of how to make sure they are stable before merging. |
FrankChen021
left a comment
There was a problem hiding this comment.
LGTM. This matches both ways testSupervisorIngestsOnlySelectedPartitions failed on master on 2026-10-16. Both runs passed on retry, so neither job failed:
- Line 130, ac864bf (#20583), job 116764398036: the first
SELECT COUNT(*)returned400 invalidInput: Object 'datasource_fpedfoba' not found. That is the Broker-schema race in the description:ingest/events/processedfired on the Indexer before the Broker knew about the datasource. - Line 140, 17d5ad0 (#20584), job 116764225473:
expected: <2> but was: <1>after replacing the spec with partitions {1, 3}. This is CI evidence for the second race the description calls unreproduced. The cumulativeevents/processed >= 2can pass on a re-read of p1 (its offset not yet committed) or before the Broker sees the p3 segment. Waiting for handoff and then Broker queryability closes both.
What I checked:
waitForHandedOffRowswill finish:newKafkaSupervisor()usesmaxRowsPerSegment=1andtaskDuration=500ms, so each row is its own segment and publishes quickly.dataSourceis unique per test and there is one task group with no replicas, sohasSumAtLeast(2)can only count p1 and p3.- Handoff follows a transactional publish, so p1's offset is committed before the spec is replaced and the new task can't re-read it.
submitSupervisorupdatessupervisorSpec, so@AfterEachsuspends the expanded spec. Removing the trailingcreateSuspendedSpec()is safe, and cleanup now also runs when an assertion fails.- The
EmbeddedClusterApisrefactor keepswaitForAllSegmentsToBeQueryable's behaviour. The>=inwaitForSegmentsToBeQueryableis right for realtime segments that aren't in the metadata store yet. - CI is green on
ad39f699, including theE*,G*,F*andK*jobs. - Locally,
EmbeddedKafkaSupervisorTestpassed 5 of 5 tests in each of 5 back-to-back runs onad39f699with-Dsurefire.rerunFailingTestsCount=0(JDK 25, Colima, about 24–25 s per run).
Nit, not blocking: the description could link the line-140 failure above as evidence for the second race.
Review prepared with Claude Code (AI-assisted); I checked the findings before approving.
Description
EmbeddedKafkaSupervisorTest.testSupervisorIngestsOnlySelectedPartitions(added in #20474) sometimes fails its first SQL check withObject 'datasource_xxx' not found. It failed all 4 attempts in one downstream CI job. Two other tests in the class have the same pattern, so this PR fixes them too.Root cause
The test waited for the Indexer's
ingest/events/processedmetric and then queried the Broker. That metric only means the task read the row. The Broker may not have added the datasource to its SQL schema yet.The second half of the test has a related race, which Claude found by reading the code and I have not reproduced. Replacing the supervisor spec stops the old task without publishing. If its partition 1 offset was not committed yet, the new task reads partition 1 again, and that alone can satisfy the cumulative wait before partition 3 arrives.
Fix
EmbeddedClusterApis.waitForSegmentsToBeQueryable(dataSource, numSegments, timeoutMillis). It works likewaitForAllSegmentsToBeQueryablebut takes the segment count, so it also covers realtime segments that are not published yet.waitForAllSegmentsToBeQueryablenow calls it.testSupervisorIngestsOnlySelectedPartitionsandtest_runKafkaSupervisorWithHeaderFilteringshare awaitForHandedOffRowshelper: wait for handoff, then for the Broker to see the segments. Handoff happens after publish, so the offsets are committed before the spec is replaced. The selected-partitions test also usessubmitSupervisornow, so the@AfterEachteardown cleans up after a failure.test_runKafkaSupervisorchecks the row count while ingestion is still running, so it waits for the Broker to see all 10 segments instead of waiting for handoff.Related
Earlier fixes in this class: #19581 (the
events/processedwait this PR replaces), #20479 (the@AfterEachteardown) and #20519 (waiting for handoff without suspending the supervisor).This PR has: