fix(inkless:ci): green the upstream test suite (diskless test fixes + flaky quarantine)#700
Open
jeqo wants to merge 3 commits into
Open
fix(inkless:ci): green the upstream test suite (diskless test fixes + flaky quarantine)#700jeqo wants to merge 3 commits into
jeqo wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Updates upstream Kafka unit tests in the Inkless fork to reflect Diskless Topics-related behavioral changes (string formatting, ApiKey ordering, and additional JMX MBeans due to topicType-tagged metrics), restoring CI green without modifying production code.
Changes:
- Adjust
PartitionRegistrationexpectedtoString()output to include diskless-related fields. - Make
NodeApiVersionsTestuse a guaranteed-last unknown apiKey (Short.MAX_VALUE) now that higher-valued diskless ApiKeys exist. - Update broker topic metrics test to account for diskless-tagged all-topics BytesIn/BytesOut being registered as additional JMX MBeans.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| metadata/src/test/java/org/apache/kafka/image/node/TopicImageNodeTest.java | Updates expected partition registration string to include diskless fields. |
| clients/src/test/java/org/apache/kafka/clients/NodeApiVersionsTest.java | Ensures the “unknown apiKey sorts last” assertion remains stable with new ApiKeys. |
| core/src/test/scala/unit/kafka/metrics/MetricsTest.scala | Adjusts assertion to account for diskless-tagged duplicate all-topics metrics in JMX. |
Three upstream Kafka unit tests broke on main because they were not adapted to Inkless changes: - TopicImageNodeTest.testChildPartitionId: PartitionRegistration.toString now includes the diskless fields (classicToDisklessStartOffset, disklessProducerStates, disklessLeaderEpoch); update expected string. - NodeApiVersionsTest.testUnknownApiVersionsToString: the "unknown" apiKey 337 no longer sorts last once the diskless client APIs (500/501) exist, so toString no longer ends with it; use Short.MAX_VALUE instead. - MetricsTest.testGeneralBrokerTopicMetricsAreGreedilyRegistered: all-topics BytesIn/BytesOut are registered for both topicType=classic and diskless. The diskless variants share a metric name with their classic counterparts, so they are extra JMX MBeans but not extra metricMapKeySet() entries; account for the diskless-tagged duplicates in the assertion. Test-only changes; no production code touched.
jeqo
force-pushed
the
jeqo/fix-upstream-ci
branch
from
July 15, 2026 08:57
bb94c1d to
1045633
Compare
…ith @flaky These integration tests are documented-flaky on upstream Apache Kafka (several with currently-open flaky-test JIRA tickets naming the exact methods/params) and reddened the noflaky-nonew CI bucket independent of any Inkless change. Tag them @flaky so they are excluded from the main suite and run only in the flaky bucket (with retries), while tracking the upstream tickets. MM2 integration family (tagged once on the base methods in MirrorConnectorsIntegrationBaseTest; inherited by IdentityReplication, ExactlyOnce, SSL, Transactions, CustomForwardingAdmin): - testReplicateSourceDefault -> KAFKA-15926 (open; also KAFKA-15927) - testReplicateTargetDefault -> KAFKA-15926 (no dedicated ticket; same harness) - testSyncTopicConfigs -> KAFKA-15945 (also KAFKA-15523, KAFKA-14971) - testReplication -> KAFKA-16488 (also KAFKA-12566) Standalone: - MonitorableSinkIntegrationTest.testMonitorableSinkConnectorAndTask -> KAFKA-18952 - TaskAssignorConvergenceTest.randomClusterPerturbationsShouldConverge -> KAFKA-16491 (open) - ShareConsumerTest.testPollThrowsInterruptExceptionIfInterrupted -> KAFKA-19961 (open) - SaslOAuthBearerSslEndToEndAuthorizationTest.testNoConsumeWithoutDescribeAclViaSubscribe -> KAFKA-9655; overridden solely to tag the OAUTHBEARER variant so other SASL mechanisms keep upstream coverage. build.gradle: add testImplementation test-common-util to :streams and :connect:mirror (source of the @flaky annotation; the other affected modules already depend on it). Verified: compile + checkstyle pass for all touched modules, and the flaky filter excludes the tagged tests from the noflaky bucket (inherited MM2 method and the SASL override both report "No tests found" in default mode).
The flaky/quarantine matrix job runs only @Flaky-tagged tests (known-unreliable upstream tests, e.g. the MM2 config-sync integration tests that pass in isolation but flake under CI load). Previously junit.py failed the job on any non-zero Gradle exit code, so a flaked quarantined test turned the whole workflow red even though nothing at the Kafka coverage level was broken - defeating the point of quarantine and making the signal untrustworthy. Make junit.py treat a non-zero Gradle exit code as non-fatal when running the flaky job (RUN_FLAKY=true), while still failing on real infrastructure problems: timeouts (exit 124), thread dumps (hangs), and a missing exit code. Flaky failures are still reported in the job summary tables. The noflaky and new jobs are unaffected and remain the trustable gating signal. Net effect: the workflow is green unless a non-flaky test breaks or infra fails. Verified junit.py exit behavior locally: - flaky + test failures -> exit 0 (green) - noflaky + test failures -> exit 1 (red) - flaky + timeout (124) -> exit 1 (infra, still red) - clean / unset RUN_FLAKY -> unchanged
Contributor
Author
jeqo
marked this pull request as ready for review
July 16, 2026 07:43
| } | ||
|
|
||
| @Test | ||
| @Flaky(value = "KAFKA-16488", comment = "Chronically flaky MM2 integration test across all subclasses; see also KAFKA-12566.") |
Contributor
There was a problem hiding this comment.
As far as I know this annotation can't be inherited as the annotation processor doesn't walk the method hierarchy. Did your test flag this as flaky though?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upstream Kafka tests broken on main (see https://github.com/aiven/inkless/actions/runs/29324563467/job/87077973795) because they were not adapted to Inkless changes or are flaky.
This PR makes the JUnit test workflow a trustable signal: green unless a non-flaky test breaks or infrastructure fails. Three independent concerns, one commit each:
noflaky-nonewCI bucket independent of any Inkless change. Tagged them@Flakyso they run only in the flaky bucket while tracking the upstream tickets.Commit 1 -
fix(inkless:ci): update upstream tests for diskless additionsTopicImageNodeTest.testChildPartitionIdPartitionRegistration.toString()now includes the diskless fields (classicToDisklessStartOffset,disklessProducerStates,disklessLeaderEpoch)NodeApiVersionsTest.testUnknownApiVersionsToString337no longer sorts last once the diskless client APIs (INIT_DISKLESS_LOG=500,ALTER_DISKLESS_SWITCH=501) exist, sotoString()no longer ends with itShort.MAX_VALUEas the unknown keyMetricsTest.testGeneralBrokerTopicMetricsAreGreedilyRegisteredBytesIn/BytesOutmeters are registered for bothtopicType=classicandtopicType=diskless; the diskless variants share a metric name with their classic counterparts, so they are extra JMX MBeans but not extrametricMapKeySet()entries (22 vs 24)topicMetrics(None)onceI verified against
apache/trunkthat none of these have an upstream fix to adopt: the upstream tests are byte-identical and pass there because upstream lacks the diskless fields / API keys / dual classic+diskless meters. The divergence is entirely Inkless-introduced, so the fixes belong in the fork.Commit 2 -
test(inkless:ci): quarantine known-flaky upstream integration tests with @FlakyEach tagged test maps to an upstream Apache Kafka flaky-test JIRA ticket; in several cases the exact method (and parameter) is named, and the ticket is still open. These are not affected by this PR's diff.
MirrorConnectorsIntegrationBaseTest.testReplicateSourceDefault(inherited by 5 subclasses)MirrorConnectorsIntegrationBaseTest.testReplicateTargetDefault(inherited by 5 subclasses)MirrorConnectorsIntegrationBaseTest.testSyncTopicConfigs(inherited by 5 subclasses)MirrorConnectorsIntegrationBaseTest.testReplication(inherited by IdentityReplication)MonitorableSinkIntegrationTest.testMonitorableSinkConnectorAndTaskTaskAssignorConvergenceTest.randomClusterPerturbationsShouldConverge[balance_subtopology]ShareConsumerTest.testPollThrowsInterruptExceptionIfInterruptedSaslOAuthBearerSslEndToEndAuthorizationTest.testNoConsumeWithoutDescribeAclViaSubscribeNotes on placement:
MirrorConnectorsIntegrationBaseTest; JUnit propagates the tag to the inherited executions in all subclasses (IdentityReplication,ExactlyOnce,SSL,Transactions,CustomForwardingAdmin).testNoConsumeWithoutDescribeAclViaSubscribeis defined in the sharedEndToEndAuthorizationTestbase and inherited by ~10 auth-mechanism subclasses. To avoid silencing it for all mechanisms, it is overridden solely in the OAUTHBEARER subclass to carry the tag; every other SASL variant keeps upstream coverage.build.gradle: addedtestImplementation project(':test-common:test-common-util')(home of the@Flakyannotation) to:streamsand:connect:mirror; the other affected modules already depend on it. The annotation is a thin meta-annotation for@Tag("flaky"), whichKafkaPostDiscoveryFilterexcludes from the main suite.Commit 3 -
fix(inkless:ci): make the flaky test job report-only so CI is trustableThe flaky/quarantine matrix job runs only
@Flaky-tagged tests.junit.pypreviously failed the job on any non-zero Gradle exit code, so a single quarantined test flaking turned the whole workflow red even when nothing at the Kafka coverage level was broken. This defeats the purpose of quarantine and makes the signal untrustworthy.Change:
junit.pynow treats a non-zero Gradle exit code as non-fatal when running the flaky job (RUN_FLAKY=true, passed from the matrix inbuild.yml), while still failing on real infrastructure problems - timeouts (exit 124), thread dumps (hangs), and a missing exit code. Flaky failures are still reported in the job summary tables. The noflaky and new jobs are unaffected and remain the trustable gating signal.Net effect: the workflow is green unless a non-flaky test breaks or infrastructure fails.
Verification
compileTestJava/compileTestScala+checkstyleTestpass for every touched module.noflakybucket on two representative cases (an inherited MM2 method and the SASL override), both reporting "No tests found for given includes" in default mode; they still run in the flaky bucket with retries.junit.pyexit behavior verified locally: flaky + test failures -> exit 0; noflaky + test failures -> exit 1; flaky + timeout (124) -> exit 1; clean / unsetRUN_FLAKY-> unchanged.