Repository navigation
Conversation
…mage The integration suite is the evidence behind this project's reliability claims, so the broker it runs on should be the one the claims are about: Apache Kafka, not a vendor distribution of it. Deleting the version translation is the real win. `BrokerIntegrationTest` read the Kafka client version off the classpath and converted it into a Confluent Platform tag - CP major = AK major + 4, patch forced to `.0`, so AK 3.9.2 meant `cp-kafka:7.9.0`. The `apache/kafka` images are named for the Kafka release they ship, so the conversion is gone rather than reimplemented: the tag IS the version, the exact client patch is now what the broker runs, and a `-Dkafka.version` bump needs no second mapping kept in step with it. Below 3.7.0 no such image was ever published, so the fallback says loudly that the pair under test is not the pair that was asked for. The container comes from `org.testcontainers.kafka.KafkaContainer`, in the `org.testcontainers:kafka` module already on the classpath at the project's Testcontainers version - no dependency change needed. Two broker settings are dropped because that container applies exactly the same values, and `group.initial.rebalance.delay.ms` is deliberately held at the value the Confluent image was configured with rather than taking the container's faster default, so changing image does not also silently retune consumer-group formation. `BrokerImageTest` is as much the point as the swap is: it asserts against the container that actually started and the config the broker actually reports, so a change that silently failed to take effect cannot pass as one that worked. ## The behavioural difference this exposed The Apache image is KRaft; the Confluent one ran an embedded ZooKeeper. Three untouched tests went red on the first full run, all with the same shape - a producer future expiring after exactly `delivery.timeout.ms`, nowhere near the behaviour under test. `createTopics(..).all()` completing means the controller accepted the topic, not that a leader has been elected for each partition and is serving. Produce into that window and the broker answers `NOT_LEADER_OR_FOLLOWER` - retriable, so it looks harmless. It is not harmless to an idempotent producer: the rejected in-flight batches come back out of order, sequence numbers desynchronise, and it spins on `OUT_OF_ORDER_SEQUENCE_NUMBER` until delivery times out. The window exists under ZooKeeper too, but is wide enough under KRaft to lose. So `KafkaClientUtils#createTopicsBlocking` now waits for every partition to answer a request only its leader can serve, which is what its javadoc already promised. Fixed in the shared helper rather than in the three tests that lost the race, because any test that creates a topic and produces to it can lose it. Measured, with a control arm, not assumed. The full lane on the Confluent image showed zero occurrences of the trigger; on the Apache image without the wait, 25 per run and three failures. An earlier, weaker version of the wait that only asked whether a leader was *assigned* took it to 2, not 0 - the broker reports the assignment before it has finished becoming the leader - and would have shipped a flake that came back under load. The write-up is in `docs/solutions/`. Two latent defects surfaced on the way. The fallback image constant sat below the static container field that reads it, so the fallback path NPE'd on the one occasion it existed for. And the ArchUnit rule keeping Docker-using tests out of the unit suite named only `org.testcontainers.containers`, so the modern `org.testcontainers.kafka` containers could have reached surefire unnoticed; it now names both.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its |
…hanges) Directory move only, so every path is 100% similar and git's exact-rename detection cannot fail on it. The content edits follow in the next commit. Generated by bin/rename-packages.sh.
Text edits only. No file moves in this commit, so it cannot dilute the rename detection in its parent. Generated by bin/rename-packages.sh.
…roker-image # Conflicts: # .idea/runConfigurations/All_examples.xml # .idea/runConfigurations/_Tag__transactions__.xml # README.adoc # bin/rename-packages.sh # parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/internal/Documentation.java # parallel-consumer-core/src/test-integration/java/bz/stub/parallelconsumer/integrationTests/utils/KafkaClientUtils.java # parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/TestConventionRules.java # parallel-consumer-core/src/test/resources/logback-test.xml # parallel-consumer-examples/parallel-consumer-example-core/src/test/resources/logback-temp-test.xml # parallel-consumer-examples/parallel-consumer-example-reactor/src/test/resources/logback-test.xml # parallel-consumer-examples/parallel-consumer-example-streams/src/test/resources/logback-test.xml # parallel-consumer-examples/parallel-consumer-example-vertx/src/test/resources/logback-test.xml # parallel-consumer-mutiny/src/test/resources/logback-test.xml # parallel-consumer-reactor/src/test/resources/logback-test.xml # parallel-consumer-vertx/src/test/resources/logback-test.xml # src/docs/README_TEMPLATE.adoc
…roker guard honest Review of the apache/kafka swap found that two of its own guarantees were narrower than stated. Neither was failing; both were passing for the wrong reason. `createTopicsBlocking` waits for a partition leader so a producer cannot enter the leaderless window - but a test that never created its topic got one anyway, because the broker auto-creates on first produce. `TransactionTimeoutsTest` does exactly that with its `-output` topic, and it is a transactional producer, which is the worst case in the write-up this change came from. Auto-creation is now off, so `createTopicsBlocking` is the only way to get a topic and the wait is an invariant rather than advice: the same mistake now fails at the call site with UNKNOWN_TOPIC_OR_PARTITION instead of expiring a producer two minutes later. The full core integration suite passes with it off, so that test held the only bypass. Inside the wait, only `listOffsets` was guarded and only for `ExecutionException`. The `describeTopics` call sat outside the try, and Awaitility does not ignore exceptions from a polled condition - so a timeout on the describe aborted the whole wait rather than polling again, which is a version of the flake being fixed. Both round trips are now inside, `TimeoutException` counts as "not ready yet", and `InterruptedException` restores the flag and abandons the wait. Per-call bounds get their own 10s constant: sharing the 60s overall budget let one stalled call spend all of it, and the failure then blamed leader election for a slow admin request. The timeout now says what it was actually waiting for. `BrokerImageTest` exists so an image change that failed to take effect cannot pass as one that worked, but it asserted on `getDockerImageName()` - the image this process ASKED for, not the one running - so it could only fail through the derivation the tests below it already cover. It now inspects the started container, which matters more than it reads: the container is created `withReuse(true)`. A tag is still a mutable label, so one assertion asks the broker its own `inter.broker.protocol.version`; that carries no patch component, and the javadoc says so rather than implying a stronger check than it makes. Also pinned `transaction.state.log.min.isr`, the one setting dropped for a container default with no assertion behind it. Kafka's own default is 2, which on a single-node broker means no transaction can commit - it would have arrived as a wall of coordinator errors nowhere near the upgrade that caused it. Two cleanups the review turned up: the arch rule listed the Testcontainers packages it knew about, which is why this change had to touch it at all when KafkaContainer moved - it now names the namespace. And the KafkaClientUtils constructor set two broker env vars on an already-started container, so they did nothing; they read as load-bearing next to settings this change deliberately delegates to the container. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
marked this pull request as draft
August 19, 2026 03:11
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
1 participant
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.
Description
Moves the integration-test broker from the Confluent Platform image to the official
apache/kafkaimage. The integration suite is the evidence behind this project's reliability claims, so the broker
it runs on should be the one the claims are about.
This is independent of the package rename and can land on its own - it touches no
io.confluent.*package declaration, only test-infrastructure code inside the existing tree.
The version translation is deleted, not reimplemented
BrokerIntegrationTestread the Kafka client version off the classpath and converted it into aConfluent Platform tag: CP major = AK major + 4, patch forced to
.0, so AK 3.9.2 meantcp-kafka:7.9.0. Theapache/kafkaimages are named for the Kafka release they ship, so the wholeconversion is gone: the tag is the version. Consequences worth having:
X.Y.0.-Dkafka.versionbump needs no second mapping kept in step with it. The CI matrix mechanism isunchanged - the version is still read at runtime from the client jar.
apache/kafkaimage was ever published, so the fallback logs loudly that thepair under test is not the pair that was asked for, instead of failing as an opaque registry error.
Container class and version
org.testcontainers.kafka.KafkaContainer, from theorg.testcontainers:kafkamodule already on theclasspath at the project's current Testcontainers version. No dependency change and no version bump
were required - that module ships both the Apache container and
ConfluentKafkaContainer, and thelegacy
org.testcontainers.containers.KafkaContainerthis replaces is the deprecated original.Two broker settings are dropped because the new container applies exactly the same values. One is kept
deliberately:
group.initial.rebalance.delay.msstays at the value the Confluent image was configuredwith rather than taking the container's faster default, so changing image does not also silently
retune consumer-group formation.
BrokerImageTest- proving the change took effectNew, and as much the point as the swap is. It asserts against the container that actually started
and the config the broker actually reports, not against the source that names them, so a change that
silently failed to take effect cannot pass as one that worked. It also pins the no-translation
property, the pre-release-suffix handling and both fallback paths.
The behavioural difference this exposed
The Apache image is KRaft; the Confluent one ran an embedded ZooKeeper. On the first full run, three
untouched tests went red -
PartitionOrderProcessingTest(both methods) andDrainingMemberRebalanceIT- all with the same shape: a producer future expiring after exactlydelivery.timeout.ms, nowhere near the behaviour under test.createTopics(..).all()completing means the controller accepted the topic, not that a leader has beenelected for each partition and is serving. Produce into that window and the broker answers
NOT_LEADER_OR_FOLLOWER- retriable, so it looks harmless. It is not harmless to an idempotentproducer, which these tests use by default: the rejected in-flight batches come back out of order,
sequence numbers desynchronise, and the producer spins on
OUT_OF_ORDER_SEQUENCE_NUMBERuntildelivery times out two minutes later under an unrelated error code. The window exists under ZooKeeper
too; it is wide enough under KRaft to lose.
KafkaClientUtils#createTopicsBlockingnow waits for every partition to answer a request only itsleader can serve, which is what its javadoc already promised. Fixed in the shared helper, not in the
three tests that lost the race, because any test that creates a topic and produces to it can lose it.
No assertion was weakened, no timeout raised, nothing quarantined.
Write-up:
docs/solutions/test-flakiness/topic-created-is-not-topic-writable-kraft-2026-08-12.md.What was verified, and how many times
All runs local, macOS, Docker 27.4.0, one full reactor
clean verifyeach viabin/ci-integration-test.sh(no-DforkCountoverride, so sequential as on GitHub-hosted runners).The two error codes are counted directly out of the run logs, because pass/fail alone cannot tell
"fixed" from "got lucky".
NOT_LEADER_OR_FOLLOWEROUT_OF_ORDER_SEQUENCE_NUMBERconfluentinc/cp-kafka:7.9.0apache/kafka:3.9.2apache/kafka:3.9.2Final state: zero failures in 3 consecutive full integration lanes (126 tests each: 123 core with
6 skipped, plus vertx, streams and metrics), and zero occurrences of either error code in any of them.
Unit lane (
bin/ci-unit-test.sh) green.A same-position control arm settles the cause rather than assuming the fix proved it - the failing
tests only, gate present versus gate absent, 3 repetitions each, everything else identical:
NOT_LEADER_OR_FOLLOWERper runA refuted prediction, reported because it nearly shipped a flake. The first version of the gate
asked
describeTopicswhether a leader was assigned - the obvious check. It took the trigger from 25per run to 2, not to 0: the broker reports the assignment before it has finished transitioning the
replica into the leader role. Had that run happened to go green, the weaker check would have merged and
the flake would have returned under load.
Broker startup, same host, images pre-pulled, 3 measurements each:
apache/kafka2.09-2.43 sversus
cp-kafka3.64-4.25 s, and the image is 408 MB versus 1.14 GB. Both matter to the CIbudget, which pays for one broker per fork. Full-lane wall clock is comparable (11:23 versus 11:48 on
the control).
Notes
BrokerIntegrationTestby file,all fork issues, all upstream issues and PRs, and the write-ups under
docs/solutions/anddocs/plans/. Nothing proposed or attempted this move. The one live reference isWIP: Java baseline + Kafka 4 support (0.7.x) #53 (Java baseline + Kafka 4), whose own risk table names "fallback to the
apache/kafkaimage" as the mitigation if no Confluent tag ships Kafka 4.2 - this PR removes thatrisk entirely, since the tag simply follows
kafka.version.field that reads it, so the fallback NPE'd on the one path it existed for; and the ArchUnit rule
keeping Docker-using tests out of the unit suite named only
org.testcontainers.containers, so themodern
org.testcontainers.kafkacontainers could have reached surefire unnoticed. Swept the treefor other instances of the Confluent-image assumption: none outside dated historical records under
docs/plans/anddocs/solutions/, which are left alone. The remainingorg.testcontainers.containersimports are Postgres, Prometheus and the log consumer - checked andunrelated.
Checklist
docs/testing.mdnow names the Apache image and the topic readiness contract;new write-up under
docs/solutions/test-flakiness/docs/features/BrokerImageTestasserts the running container and the broker's ownreported config