test: poll sys.segments in verifyNumVisibleSegmentsIs to fix flaky compaction tests

[FrankChen021]

Related to #20312 (item 7).

Description

CompactionTaskTest.testCompactionWithTimestampDimension failed on master with:

Segment count mismatch in sys.segments table ==> expected: <2> but was: <>
  at EmbeddedClusterApis.verifyNumVisibleSegmentsIs(EmbeddedClusterApis.java:290)
  at CompactionTestBase.verifySegmentsCount(CompactionTestBase.java:102)

Example: https://github.com/apache/druid/actions/runs/34443807042/job/102764139085.

The Overlord-side assertion passed (2 visible used segments). verifySegmentsCount then waited for a single segment/metadataCache/sync/time event on the Broker and asserted the sys.segments count once. A sync that was already in flight when the compaction finished can complete while still reflecting the previous state, so one sync event does not guarantee the Broker has caught up.

Changes

• EmbeddedClusterApis.verifyNumVisibleSegmentsIs keeps the immediate Overlord assertion and polls the Broker-side sys.segments count using the existing ResultWaiter (60 s deadline). On timeout it throws an AssertionError that includes the expected count, the datasource and the last observed result.
• CompactionTestBase.verifySegmentsCount no longer waits for the single sync event, since the poll covers it.

verifyNumVisibleSegmentsIs has a single caller (CompactionTestBase).

Verified locally with mvn -pl services,embedded-tests -am test-compile. The embedded compaction tests were not run locally.

---

Key changed/added classes in this PR

• EmbeddedClusterApis
• CompactionTestBase

---

This PR has:

• been self-reviewed.

[Copilot]

🟢 Approval recommended

The change is narrowly scoped to test utilities and addresses a confirmed race by replacing a single-shot assertion with a bounded poll using existing infrastructure.

Pull request overview

This PR reduces flakiness in embedded compaction tests by changing segment-count verification to tolerate the Broker’s asynchronous propagation of segment state into sys.segments.

Changes:

• Update EmbeddedClusterApis.verifyNumVisibleSegmentsIs to poll the Broker-side sys.segments count (with a bounded timeout) instead of asserting after a single read.
• Remove the “wait for one metadata cache sync metric” step from CompactionTestBase.verifySegmentsCount, relying on the new polling behavior instead.

File summaries

File	Description
services/src/test/java/org/apache/druid/testing/embedded/EmbeddedClusterApis.java	Polls sys.segments via existing ResultWaiter to avoid failing on transient Broker lag after compaction.
embedded-tests/src/test/java/org/apache/druid/testing/embedded/compact/CompactionTestBase.java	Drops the single sync-event wait since verifyNumVisibleSegmentsIs now performs bounded polling.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[FrankChen021]

🟡 Changes recommended

Reviewed 2 of 2 changed files; finding in 1 file.

The new bounded Broker poll addresses the single-shot count race, but removing the metadata-cache synchronization leaves an unchanged-count path that can accept stale Broker state. In the existing compaction flow, the second compaction changes query granularity from HOUR to MINUTE while retaining two segments, so COUNT(*) alone does not prove that the post-compaction segment metadata is visible. Preserve a relevant cache barrier or poll an identity/metadata condition that distinguishes the new segments.

Validation: git diff --check fb8bb5fafd08ea7ec2a5e83704e4c287ef5bede9 9f6d3536aea61f37dc9e93393d6e7d032c0a962c passed. No builds, tests, dependency installs, fetches, prepares, or formatters were run, per request.

Severity	Findings
P0	0
P1	0
P2	1
P3	0
Total	1

---

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

[FrankChen021]

🟡 Changes recommended

Reviewed 2 of 2 changed files.

The current ID predicate fixes the previously reported unchanged-count race by requiring the Broker to expose the current segment IDs, and I rechecked both compaction callers and the sys.segments implementation. One remaining correctness issue is that the IDs are embedded into a SQL format string before runSql applies StringUtils.format; valid percent signs in a datasource or segment ID are therefore treated as format directives, which can throw or substitute the datasource argument and make the test fail. This should be fixed before merging.

Validation: git diff --check fb8bb5fafd08ea7ec2a5e83704e4c287ef5bede9 bf951bfbe74049023ee85f8f4a1f2545fe41057b passed. No builds, tests, dependency installs, fetches, prepares, or formatters were run.

Severity	Findings
P0	0
P1	0
P2	1
P3	0
Total	1

---

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

[FrankChen021]

[P2] Percent signs in segment IDs break SQL formatting

Finding: expectedSegmentIds is inserted into sql before runSql calls StringUtils.format(sql, ...). Because segment IDs include the datasource name and Druid permits % in IDs, a datasource or version containing a format sequence such as %s or %n is interpreted by String.format on every poll rather than treated as SQL data. The helper then throws or generates a malformed or incorrect query, so compaction verification fails for otherwise valid datasource names.

Suggestion: Encode percent signs for the outer format string, for example with StringUtils.encodeForFormat after SQL escaping, or format the datasource before appending the ID literals so segment IDs never pass through StringUtils.format.