feat(kafka): add apache/kafka and apache/kafka-native support #3249
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
strowk
force-pushed
the
feature/kafka_native
branch
2 times, most recently
from
eabd044 to
b4dcd24
Compare
|
For comparison: on my machine currently, $ cd modules/kafka_native && go test -timeout 30s -count 1 -run '^TestKafka$' github.com/testcontainers/testcontainers-go/modules/kafka_native ; cd -
ok github.com/testcontainers/testcontainers-go/modules/kafka_native 3.693s
$ cd modules/kafka && go test -timeout 30s -count 1 -run '^TestKafka$' github.com/testcontainers/testcontainers-go/modules/kafka ; cd -
ok github.com/testcontainers/testcontainers-go/modules/kafka 16.654sTests themselves are identical, only different images and less confluent-dependant configs. Upd: Commands above are outdated, check PR description for up to date way to measure. |
strowk
force-pushed
the
feature/kafka_native
branch
from
b4dcd24 to
b044a54
Compare
strowk
force-pushed
the
feature/kafka_native
branch
from
b044a54 to
1ef2f84
Compare
|
Hi @strowk thanks for adding this! I'm currently taking a look, also discussing internally with the other TC maintainers if this code should live as a new sub-package under the kafka module. What are your thoughts on this? |
mdelapenya
added
the
feature
|
Sounds ok, though not sure what would interface look like |
|
Hmmm thinking more in depth about the submodule, will the existing kafka options apply to this native image? If so, then we should probably find them, although it can be probably done in a follow-up. |
|
Hi @mdelapenya and @strowk, Are we ensured we need a new module instead of extending existing one (kafka)? I planned to prepare that change - refer to main...mabrarov:testcontainers-go:feat/kafka_alt_images - since May 2025 (refer to #2683 (comment) and mabrarov/testcontainers-go-kafka-2206#2), but I got time for that just recently. Please note that (my case) usage of apache/kafka-native image can be problematic due to this image doesn't contain tools (like kafka-topics) which can be required for Kafka health check (e.g. in Docker Compose or Kubernetes). Using different images for tests (Testcontainers: apache/kafka-native) and local / dev or QA environments (Docker Compose, Kubernetes: apache/kafka) can bring additional issues. Thank you. |
|
@mabrarov , if I understand correctly some earlier concerns from maintainers were that removing confluent image from existing module would be not backwards compatible change for users that rely on something confluent specific (I for sure am one of those, albeit I know how to transition). I.e it would for those users look such that the new version is pulled by something like dependabot or renovate and then they have to figure out how to transition to new image and have to stop updating testcontainers until the solution is found, which is somewhat undesirable situation. As to bitnami.. seeing what Broadcom did recently to images and how much the secured ones cost.. I sort of doubt it would find very many users TBH. This is just my opinion of course, but I would avoid bitnami as much as possible from now on. |
|
Hi @strowk, You are right about binami/kafka image (the reason I got back to this work). As for confluentinc/confluent-local image - we can keep it too while adding support for apache/kafka and apache/kafka-native images to kafka module. Feel free to check feat/kafka_alt_images branch I referenced in my previous comment. Anyway, I'm happy (and appreciate) the work on kafka module is going on even if it will be a dedicated module. My need in that case is to support not just apache/kafka-native but apache/kafka image too. Thank you. |
|
Ooh, thanks for clarification, I did not know that there separate apache/kafka is somehow significantly better than the native one. This does mean that it would be better for users to have access to all three possible images -confluent/apache-not-native/apache-native via same module and pick between "flavors" by some sort of an option probably? |
It is a question I have too. Maybe maintainers of Testcontainers for Go can answer it? IMHO: only limited set of docker images (including tags) can be supported because we cannot predict what changes will be introduced in new version of image. Considering this limitation I'd just recommend to use specific (including tag) image with Testcontainers. Thank you. |
|
@mabrarov , I suppose that deriving what to do from parsing the image is a common answer in codebase as I can see: https://github.com/testcontainers/testcontainers-go/blob/main/modules/elasticsearch/version.go Though it is usually about version of same "flavor", but I dont see that much difference, except we probably would need to allow user to override whatever we define.. |
|
Hi @strowk, Examples you referenced in #3249 (comment) cover only lower bound while I am worried about upper bound too. Anyway, we can start from:
I feel it will be still the "Testcontainers way". Thank you. |
|
@mabrarov , I have made an attempt at this based on a very simple logic that looks like this: const (
apacheKafkaImagePrefix = "apache/kafka"
confluentincImagePrefix = "confluentinc/"
dockerIoPrefix = "docker.io/"
)
func isApache(image string) bool {
return strings.HasPrefix(image, apacheKafkaImagePrefix) || strings.HasPrefix(image, dockerIoPrefix+apacheKafkaImagePrefix)
}
func isConfluentinc(image string) bool {
return strings.HasPrefix(image, confluentincImagePrefix) || strings.HasPrefix(image, dockerIoPrefix+confluentincImagePrefix)
}
func getStarterScriptContent(image string) string {
if isApache(image) {
return apacheStarterScriptContent
} else if isConfluentinc(image) {
return confluentincStarterScriptContent
} else {
// Default to confluentinc for backward compatibility
// in situations when image was custom specified based on confluentinc
return confluentincStarterScriptContent
}
}This does not care about specific version as such, only whether or not image is confluent vs apache. In this "apache/kafka-native" and "apache/kafka" would work the same (starter script has no issues with either so far as I can tell). It seems to work on and I tried with confluent, native and usual apache with this times:
This is just one run of course, so not super representative, but kind of interesting that apache/kafka shows good timing vs confluentinc one, though still not quite as good as native. |
This should make it easier for users to start. The change attempts to steer new users to start with native image, which shows significant performance improvement especially if many containers need to be started during entire test, due to GraalVM optimizations. While new users should prefer using Kafka Native, these changes are made in a non-breaking fashion such that if image was not detected as being from Apache, it is assumed to be from Confluent, so that even custom images that used to work before should continue to work, in case if they were based on confluentinc images
|
Warning Rate limit exceeded@strowk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughDetect Kafka image family at runtime and introduce RunOption-based configuration (WithApacheFlavor, WithConfluentFlavor, WithStarterScript, WithClusterID); select appropriate starter script during post-start; update Run flow, docs, examples, tests, and benchmarks to support multiple image flavors, localhost listener, and graceful shutdown. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (examples/tests)
participant Run as kafka.Run
participant Resolver as runOptions
participant Post as post-start hook
participant Copy as copyStarterScript
participant Container as Container FS
Note over Run,Resolver: build runOptions from image + supplied RunOption(s)
Caller->>Run: Run(ctx, image, opts...)
Run->>Resolver: build runOptions(image, opts)
Run->>Post: register post-start(container, runOptions)
Post->>Copy: copyStarterScript(ctx, runOptions, container)
Copy->>Resolver: resolve starter script (getStarterScriptContent / opts)
alt WithStarterScript supplied
Resolver-->>Copy: custom starter script
else Apache image detected
Resolver-->>Copy: apache starter script
else
Resolver-->>Copy: confluent starter script (default)
end
Copy->>Container: write starter script path & set entrypoint
Container-->>Copy: ack
Post-->>Run: readiness & wait completion
Run-->>Caller: container ready / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
strowk
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
modules/kafka/version.go (1)
19-29: Consider simplifying the conditional chain.The
else if isConfluentinc(image)on line 22 could be simplified to justelsesince the final else block already defaults toconfluentincStarterScriptContent. However, the explicit check makes the logic self-documenting and clearer.Optional refactor:
func getStarterScriptContent(image string) string { if isApache(image) { return apacheStarterScriptContent - } else if isConfluentinc(image) { - return confluentincStarterScriptContent } else { // Default to confluentinc for backward compatibility // in situations when image was custom specified based on confluentinc return confluentincStarterScriptContent } }modules/kafka/examples_test.go (1)
12-109: Consider consolidating duplicate example code.All three example functions (
ExampleRun_confluentinc,ExampleRun_apacheNative,ExampleRun_apacheNotNative) are nearly identical, differing only in the image string. While separate examples may aid documentation clarity, this creates a maintenance burden.Consider extracting common logic into a helper or documenting all variants in a single example with a table showing the different images. However, if separate examples are intentional for documentation purposes (to appear distinctly in godoc), the current approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/modules/kafka.md(4 hunks)modules/kafka/examples_test.go(2 hunks)modules/kafka/kafka.go(6 hunks)modules/kafka/kafka_test.go(2 hunks)modules/kafka/version.go(1 hunks)modules/kafka/version_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
modules/kafka/examples_test.go (3)
modules/kafka/kafka.go (2)
Run(55-129)WithClusterID(159-165)cleanup.go (1)
TerminateContainer(97-108)log/logger.go (1)
Printf(47-49)
modules/kafka/kafka.go (1)
container.go (1)
Container(41-73)
modules/kafka/version_test.go (1)
modules/kafka/kafka.go (1)
Run(55-129)
modules/kafka/kafka_test.go (1)
modules/kafka/kafka.go (2)
Run(55-129)WithClusterID(159-165)
🪛 LanguageTool
docs/modules/kafka.md
[style] ~58-~58: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 3313 characters long)
Context: ...ound(), "apache/kafka-native:4.0.1")`. !!! warning Module expects that the ima...
(EN_EXCESSIVE_EXCLAMATION)
🔇 Additional comments (13)
modules/kafka/version.go (1)
11-17: LGTM! Image detection logic is correct.The predicates correctly identify Apache and Confluent images, including handling for the
docker.io/prefix. The Apache check covers bothapache/kafkaandapache/kafka-nativeimages as intended.modules/kafka/version_test.go (2)
71-115: LGTM! Comprehensive test coverage.The test thoroughly covers various image formats including tags, no tags, and the
docker.io/prefix for both Confluent and non-Confluent images.
117-171: LGTM! Good coverage of starter script selection.The test validates the correct starter script content is returned for Apache, Confluent, and custom images across different tag formats.
modules/kafka/kafka_test.go (2)
15-67: LGTM! Good extraction of common test logic.The
testForhelper function effectively parameterizes the test by image, enabling consistent testing across multiple Kafka image variants.
69-93: LGTM! Effective use of table-driven tests.The table-driven approach ensures all three Kafka image variants (Confluent, Apache native, Apache non-native) are tested with identical assertions, improving test coverage and maintainability.
modules/kafka/kafka.go (4)
86-94: LGTM! Correct integration of image-based script selection.The
copyStarterScriptcall now receives the image parameter (line 88), enabling dynamic selection of the appropriate starter script viagetStarterScriptContent(img).
132-157: LGTM! Clean integration of dynamic script selection.The function signature now accepts the image parameter (line 132) and uses
getStarterScriptContent(img)(line 150) to select the appropriate starter script content. The rest of the logic remains unchanged.
200-227: Verify the version validation logic change is intentional.The validation logic now only checks Confluent images (line 210) using
isConfluentinc(image). Apache images and custom images bypass version validation entirely (lines 210-215).This aligns with the documentation stating "All Apache images support Kraft mode," but it means custom images based on Confluent that don't match the
confluentinc/prefix will also skip validation.Confirm this behavior is intentional:
- Apache images: Skip validation (assuming all versions support KRaft)
- Custom images: Skip validation (may or may not support KRaft)
- Only
confluentinc/*images: Validate version >= 7.4.0Consider whether custom Confluent-based images should also be validated or if the current logic is acceptable.
22-40: Based on my web search findings, the official Apache Kafka Dockerfile copies Docker scripts to/etc/kafka/docker/, which confirms the startup script path used in the code is correct. The Apache starter script's reference to/etc/kafka/docker/runaligns with the official image structure.No actionable issues found. The Apache starter script path is valid.
The code correctly uses
exec /etc/kafka/docker/run, which is the proper entrypoint for Apache Kafka official Docker images. Theexeccommand is intentional for correct process management, and the KAFKA_ADVERTISED_LISTENERS format matches the Apache image requirements.docs/modules/kafka.md (4)
7-10: LGTM! Clear introduction reflecting multi-image support.The introduction now clearly states the module supports both Apache Kafka and Confluent images, accurately reflecting the code changes.
22-33: LGTM! Comprehensive examples covering all image variants.The documentation now references three distinct examples (Apache Native, Apache non-native, Confluent), with a helpful note about the native container's performance benefits. This provides clear guidance for users choosing between image types.
60-62: LGTM! Clear guidance on KRaft mode requirements.The warning appropriately distinguishes between Confluent images (minimum version 7.4.0 required) and Apache images (all versions support KRaft), aligning with the validation logic in
validateKRaftVersion.
74-87: LGTM! Clear explanation of starter script selection.The Init script section effectively documents the image-based script selection logic, showing how the module varies the starter script based on the image prefix. The code references help users understand the implementation.
|
Hi @strowk, Just to complete my work with some milestone I opened #3488. Please feel free to discuss with maintainers what way to proceed - copy some parts of that PR (e.g. links to GitHub issues, test for graceful shutdown of Kafka) into this PR (you was the first 😄) or just complete with this PR... or maybe complete/merge #3488 instead of this PR. Thank you for your help and work. |
|
Hey @mdelapenya , I have made a lot of changes to this:
@mabrarov , as requested this should work for "apache/kafka" as well as for "apache/kafka-native" , there are tests and examples to confirm. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/modules/kafka.md(5 hunks)modules/kafka/benchmark_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-18T08:24:27.479Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3254
File: .github/dependabot.yml:21-21
Timestamp: 2025-09-18T08:24:27.479Z
Learning: In the testcontainers-go repository, submodules like atlaslocal that are part of a parent module (e.g., mongodb) share the same go.mod file and should not have separate Dependabot entries. They are already monitored through the parent module's Dependabot configuration entry.
Applied to files:
docs/modules/kafka.md
📚 Learning: 2025-09-29T13:57:14.636Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
Applied to files:
docs/modules/kafka.md
🧬 Code graph analysis (1)
modules/kafka/benchmark_test.go (2)
log/logger.go (1)
Printf(47-49)cleanup.go (1)
TerminateContainer(97-108)
🪛 LanguageTool
docs/modules/kafka.md
[style] ~71-~71: Using many exclamation marks might seem excessive (in this case: 9 exclamation marks for a text that’s 5976 characters long)
Context: ...ound(), "apache/kafka-native:4.0.1")`. !!! warning Module expects that the ima...
(EN_EXCESSIVE_EXCLAMATION)
🔇 Additional comments (5)
modules/kafka/benchmark_test.go (1)
28-38: LGTM! Good coverage of different Kafka image flavors.The three benchmark functions provide good coverage for measuring start/stop performance across Confluent, Apache, and Apache Native images, which aligns well with the PR's objectives and the comparison table in the documentation.
docs/modules/kafka.md (4)
22-47: Excellent documentation of the three Kafka image options.The usage examples and comparison table provide clear guidance on the trade-offs between Apache Native, Apache, and Confluent images. The size/performance comparison and the note about missing CLI tools in apache/kafka-native are particularly helpful for users making an informed choice.
85-110: Clear explanation of starter script selection and override options.The documentation clearly explains the automatic detection logic (with sensible defaults for backward compatibility) and provides appropriate warnings about custom scripts. The code examples effectively demonstrate the override options.
147-157: Useful addition documenting the localhost listener feature.This new section effectively explains how to use the localhost listener for custom wait strategies and CLI commands. The note about incompatibility with apache/kafka-native images is important and consistent with earlier documentation.
71-71: Static analysis false positive - ignore LanguageTool hint.The LanguageTool hint about excessive exclamation marks is a false positive. It's incorrectly counting the markdown admonition syntax (
!!! warning) as punctuation rather than recognizing it as mkdocs-material directive syntax.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/modules/kafka.md (2)
85-98: Verify starter script selection logic is complete.The documentation describes the starter script selection logic (lines 85-98), but doesn't mention whether the default behavior is to auto-detect based on image prefix or if it requires explicit options. Clarify whether:
- Auto-detection happens when using Run() without WithApacheFlavor/WithConfluentFlavor?
- What happens if detection fails to match (e.g., custom registry image)?
The text states "otherwise: use Confluent starter script (for backward compatibility)" at line 93, which addresses the third case. This appears correct, but may benefit from a small example showing the auto-detection in action.
147-157: Clarify localhost listener availability across image types.Line 149 states the Kafka container "would by default be configured with
localhost:9095" but doesn't indicate whether this applies to all three supported image types (Apache Native, Apache, Confluent) or only some. Given that line 157 notes apache/kafka-native lacks CLI tools, it's unclear if localhost:9095 is available on that image or if the listener exists but cannot be used for CLI commands.Recommend: Clarify whether localhost:9095 is configured for all images, and update the example or note to make the limitation(s) explicit per image type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/modules/kafka.md(5 hunks)modules/kafka/benchmark_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/kafka/benchmark_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-18T08:24:27.479Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3254
File: .github/dependabot.yml:21-21
Timestamp: 2025-09-18T08:24:27.479Z
Learning: In the testcontainers-go repository, submodules like atlaslocal that are part of a parent module (e.g., mongodb) share the same go.mod file and should not have separate Dependabot entries. They are already monitored through the parent module's Dependabot configuration entry.
Applied to files:
docs/modules/kafka.md
📚 Learning: 2025-09-29T13:57:14.636Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
Applied to files:
docs/modules/kafka.md
🪛 LanguageTool
docs/modules/kafka.md
[style] ~71-~71: Using many exclamation marks might seem excessive (in this case: 9 exclamation marks for a text that’s 6061 characters long)
Context: ...ound(), "apache/kafka-native:4.0.1")`. !!! warning Module expects that the ima...
(EN_EXCESSIVE_EXCLAMATION)
🔇 Additional comments (3)
docs/modules/kafka.md (3)
111-129: Verify all new options are documented.The PR summary and AI-generated summary mention four new RunOption-based configuration options:
WithApacheFlavor,WithConfluentFlavor,WithStarterScript, andWithClusterID. The documentation covers the first three (lines 115-129), butWithClusterIDis not documented in this file. Either:
- Update the documentation to include
WithClusterIDusage and examples, or- Confirm that
WithClusterIDis not part of the public API and should not be documented.
1-157: Overall documentation structure and quality look solid.The updated documentation successfully covers the addition of Apache Kafka image support alongside the existing Confluent support. Key strengths:
- Examples: Three concrete usage patterns covering all three image types (lines 22-31)
- Guidance: Excellent comparison table (lines 39-43) with practical information about trade-offs (size, startup time, CLI tools, graceful shutdown)
- API clarity: Run function signature and image selection logic are well-documented
- Options: New flavor and starter script options are documented with examples
- Transparency: Important caveats about CLI tools and graceful shutdown are highlighted
The documentation aligns well with the PR's goal of supporting multiple image families while preserving backward compatibility.
22-31: All codeinclude block references verified and correctly named.All 10 code blocks referenced in the documentation have been confirmed to exist in the source files with proper markers:
- examples_test.go:
runKafkaContainerApacheNative,runKafkaContainerApacheNotNative,runKafkaContainerConfluentinc,runKafkaContainerWithApacheFlavor,runKafkaContainerWithConfluentFlavor,runKafkaContainerAndUseLocalhostListener- kafka.go:
starterScriptApache,starterScriptConfluentinc,envVars- kafka_test.go:
getBrokersAll blocks use the correct marker format (
// blockName {...// }) that the codeinclude plugin expects.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/modules/kafka.md (2)
157-167: Clarify why localhost listener doesn't work with apache/kafka-native.Line 167 states the localhost listener example "will not work with
apache/kafka-nativeimages, as they do not include CLI tools." This is accurate but could be strengthened by briefly explaining why (users cannot runkafka-topics.shcommands inside the container). Consider mentioning that apache/kafka (non-native) does include CLI tools and thus supports this pattern.Consider revising lines 157-167 to:
Kafka container would by default be configured with `localhost:9095` as one of advertised listeners. This can be used when you need to run CLI commands inside the container, for example with custom wait strategies or to prepare test data. Here is an example that uses custom wait strategy that checks if listing topics works: <!--codeinclude--> [Custom wait strategy](../../modules/kafka/examples_test.go) inside_block:runKafkaContainerAndUseLocalhostListener <!--/codeinclude--> Note: this will not work with `apache/kafka-native` images, as they do not include CLI tools like `kafka-topics.sh`. However, it works with `apache/kafka` and `confluentinc/confluent-local` images.
73-82: Minor: Consider softening absolute claims about Apache image compatibility.While line 81 "All Apache images support Kraft mode." is likely accurate for current versions, consider whether future-proofing the documentation with language like "All currently maintained Apache Kafka images support Kraft mode" would be better for long-term clarity. This matches the pattern used elsewhere in documentation.
This is a minor stylistic suggestion; the current wording is acceptable if you're confident it will remain accurate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/modules/kafka.md(5 hunks)modules/kafka/benchmark_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/kafka/benchmark_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-18T08:24:27.479Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3254
File: .github/dependabot.yml:21-21
Timestamp: 2025-09-18T08:24:27.479Z
Learning: In the testcontainers-go repository, submodules like atlaslocal that are part of a parent module (e.g., mongodb) share the same go.mod file and should not have separate Dependabot entries. They are already monitored through the parent module's Dependabot configuration entry.
Applied to files:
docs/modules/kafka.md
📚 Learning: 2025-09-29T13:57:14.636Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
Applied to files:
docs/modules/kafka.md
🔇 Additional comments (1)
docs/modules/kafka.md (1)
105-115: Code block references are valid and all markers exist.The documentation references three code blocks using the
inside_block:format:
starterScriptApachein kafka.go (line 34) ✓starterScriptConfluentincin kafka.go (line 22) ✓runKafkaContainerWithApacheFlavorin examples_test.go (line 114) ✓All blocks are properly marked with the codebase's marker format (
// blockName {and// }), so the documentation include references will resolve correctly.
| dockerIoPrefix = "docker.io/" | ||
| ) | ||
|
|
||
| func isApache(image string) bool { |
There was a problem hiding this comment.
question: do we need both? If not each use would usually be a switch not an if which I saw above.
Alternatively it could be a imageType call returning an enum type
type flavour int
const (
unknownFlavour flavour = iota
apacheFlavour
confluentFlavour
customFlavour
)There was a problem hiding this comment.
suggestion: align filename with behaviour of the methods, type, flavour or other depending on the outcome of the other comments.
| apacheStarterScriptContent = `#!/bin/bash | ||
| export KAFKA_ADVERTISED_LISTENERS=%s,BROKER://%s:9092 | ||
| echo Starting Apache Kafka | ||
| exec /etc/kafka/docker/run` |
There was a problem hiding this comment.
@mabrarov to confirm my understanding you're suggesting using the detail from inspect to identify the location of binary to exec, so that if that changed in the future it would remain compatible?
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
This PR looks good to me, and it comes with a lot of work, thank you so much for dedicating your time for this. We'll make sure to make it into the project the soonest 🙇
Let's address the existing review comments, and let's merge it on green CI.
docs/modules/kafka.md
Outdated
| [Get Kafka brokers](../../modules/kafka/kafka_test.go) inside_block:getBrokers | ||
| <!--/codeinclude--> | ||
| ## Localhost listener |
There was a problem hiding this comment.
| ## Localhost listener | |
| #### Localhost listener |
There was a problem hiding this comment.
Hey, I've applied this without looking, but results do not seem correct to me..
This looks like "Localhost listener" section is under "Container Methods", but it does not actually describe container method, it just tells that there is a configuration that allows to do particular things from inside of container (via some options, but not via container methods).
Did you mean to put it under "Module Reference" maybe? I.e with three # symbols?
Co-authored-by: Manuel de la Peña <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/modules/kafka.md(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-18T08:24:27.479Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3254
File: .github/dependabot.yml:21-21
Timestamp: 2025-09-18T08:24:27.479Z
Learning: In the testcontainers-go repository, submodules like atlaslocal that are part of a parent module (e.g., mongodb) share the same go.mod file and should not have separate Dependabot entries. They are already monitored through the parent module's Dependabot configuration entry.
Applied to files:
docs/modules/kafka.md
📚 Learning: 2025-09-29T13:57:14.636Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
Applied to files:
docs/modules/kafka.md
🪛 LanguageTool
docs/modules/kafka.md
[style] ~77-~77: Using many exclamation marks might seem excessive (in this case: 9 exclamation marks for a text that’s 6500 characters long)
Context: ...ound(), "apache/kafka-native:4.0.1")`. !!! warning Module expects that the ima...
(EN_EXCESSIVE_EXCLAMATION)
🔇 Additional comments (4)
docs/modules/kafka.md (4)
19-39: Content is accurate and well-structured.The Apache Kafka images section correctly documents the three flavors, their trade-offs (size, startup time, CLI tools, graceful shutdown), and provides clear guidance to prefer Apache images. The comparison table and graceful shutdown callout (addressing issue #2206) are helpful for users.
155-166: Localhost listener documentation is clear and appropriately caveat-ed.The explanation of the localhost listener feature (localhost:9095) for CLI commands inside the container is helpful. The caveat about incompatibility with native images (due to missing CLI tools) is important and well-placed at the end.
119-137: All example code blocks are properly defined and correctly referenced.Verification confirms that all three code blocks referenced in the documentation exist and are properly marked in
modules/kafka/examples_test.go:
runKafkaContainerWithApacheFlavor(line 114)runKafkaContainerWithConfluentFlavor(line 154)runKafkaContainerAndUseLocalhostListener(line 197)All documentation references are valid.
42-52: All three example code blocks are properly defined and marked.Verification confirms that
modules/kafka/examples_test.gocontains all three referenced blocks:
runKafkaContainerConfluentinc(lines 15–31)runKafkaContainerApacheNative(lines 48–64)runKafkaContainerApacheNotNative(lines 81–97)Each block is properly marked with comment-style markers (
// blockname {and// }), matching theinside_block:references in the documentation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
modules/kafka/kafka_test.go (1)
174-212: Consider removingStopTimeout(0)for consistency.The test correctly validates that the localhost listener (port 9095) is accessible using an exec-based wait strategy. However, line 208 uses
testcontainers.CleanupContainer(t, kafkaContainer, testcontainers.StopTimeout(0)), which immediately stops the container without waiting. Other tests in this file (e.g., lines 25, 153) useCleanupContainerwithout an explicit stop timeout, allowing the default graceful shutdown.For consistency and maintainability, consider using the default cleanup behavior unless there's a specific reason to force immediate shutdown.
Apply this diff to align with other tests:
- testcontainers.CleanupContainer(t, kafkaContainer, testcontainers.StopTimeout(0)) + testcontainers.CleanupContainer(t, kafkaContainer)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modules/kafka/benchmark_test.go(1 hunks)modules/kafka/kafka_test.go(3 hunks)modules/kafka/options.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- modules/kafka/benchmark_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/kafka/options.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-18T08:24:27.479Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3254
File: .github/dependabot.yml:21-21
Timestamp: 2025-09-18T08:24:27.479Z
Learning: In the testcontainers-go repository, submodules like atlaslocal that are part of a parent module (e.g., mongodb) share the same go.mod file and should not have separate Dependabot entries. They are already monitored through the parent module's Dependabot configuration entry.
Applied to files:
modules/kafka/kafka_test.go
📚 Learning: 2025-09-29T13:57:14.636Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
Applied to files:
modules/kafka/kafka_test.go
🧬 Code graph analysis (1)
modules/kafka/kafka_test.go (6)
modules/kafka/kafka.go (1)
Run(55-140)modules/kafka/options.go (4)
WithClusterID(54-58)WithApacheFlavor(66-75)WithConfluentFlavor(81-90)WithStarterScript(30-39)testing.go (1)
CleanupContainer(91-97)options.go (1)
WithWaitStrategy(366-368)wait/exec.go (1)
NewExecStrategy(30-37)cleanup.go (1)
StopTimeout(76-80)
🔇 Additional comments (4)
modules/kafka/kafka_test.go (4)
17-71: LGTM!The parameterized
testForhelper cleanly extracts the core Kafka test logic and correctly usest.Helper()for proper test failure attribution. The test flow (setup → broker interaction → produce/consume → assertions) is well-structured.
73-109: LGTM!The table-driven approach provides excellent coverage across Confluent and Apache image variants (native and non-native, multiple versions). All test case names are unique and descriptive, addressing the earlier duplicate-name issue.
135-172: LGTM!The graceful shutdown test correctly validates that Apache Kafka images (which use
execto replace the shell) handle SIGTERM properly. The test captures and checks theStop()error and uses appropriate timeouts. Testing only Apache images is intentional, as this is where the signal-handling improvement applies.
214-234: LGTM!The test correctly validates that specifying conflicting flavor options (
WithApacheFlavor,WithConfluentFlavor,WithStarterScript) returns an error. The three test scenarios comprehensively cover all conflict cases, ensuring the mutual-exclusivity constraint is enforced at the API level.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
modules/kafka/version_test.go (1)
9-115: Consider adding edge case tests for defensive coverage.The current tests comprehensively cover the documented prefix-matching behavior. To make the test suite more robust, consider adding test cases for edge conditions:
- Empty string image name
- Malformed image names (e.g., no slash, only tag)
- Alternative registry prefixes (e.g., "registry.example.com/apache/kafka", "localhost:5000/confluentinc/cp-kafka")
These would verify that the detection logic handles unexpected inputs gracefully.
Example additional test cases:
{ name: "empty string", image: "", want: false, }, { name: "malformed - no slash", image: "kafka", want: false, }, { name: "custom registry with apache path", image: "registry.example.com/apache/kafka:latest", want: false, // or true, depending on intended behavior },modules/kafka/options_test.go (1)
56-65: Consider testing error path for WithStarterScript.The test verifies the happy path where
WithStarterScriptsuccessfully overrides the starter script content. However, it doesn't test the error case whereWithStarterScriptis called whenflavorWasSetis already true, which should returnerrFlavorAlreadySetaccording to the implementation inoptions.go. While the current coverage is adequate for basic functionality, adding an error-path test would improve robustness.Example:
func TestWithStarterScript_flavorAlreadySet(t *testing.T) { opts := &runOptions{ image: "apache/kafka-native:latest", flavorWasSet: true, } err := WithStarterScript("custom")(opts) require.Error(t, err) require.Equal(t, errFlavorAlreadySet, err) }docs/modules/kafka.md (2)
159-171: Add a clarification note about CLI tools for apache/kafka-native in the Localhost listener section.Line 171 notes that the localhost listener "will not work with
apache/kafka-nativeimages, as they do not include CLI tools." However, this note is somewhat cryptic—it should clarify that the localhost listener itself works, but executing CLI commands within the container (likekafka-topics.shfor custom wait strategies) will fail due to missing tools.Consider rephrasing line 171 to:
Note: CLI commands (e.g.,
kafka-topics.sh) used in custom wait strategies will not work withapache/kafka-nativeimages, as they do not include CLI tools.
27-28: Recommendation wording is strong; clarify that graceful shutdown is a known limitation.Line 27 strongly recommends preferring Apache Kafka over Confluent due to the graceful shutdown issue. While this aligns with the PR objectives and prior discussion, the wording could benefit from a slight softening to indicate this is a known limitation rather than suggesting Confluent is unsuitable. The current wording is acceptable but consider whether "Apache Kafka images are recommended due to improved graceful shutdown support" might better balance the messaging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/modules/kafka.md(5 hunks)modules/kafka/options_test.go(1 hunks)modules/kafka/version_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-18T08:24:27.479Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3254
File: .github/dependabot.yml:21-21
Timestamp: 2025-09-18T08:24:27.479Z
Learning: In the testcontainers-go repository, submodules like atlaslocal that are part of a parent module (e.g., mongodb) share the same go.mod file and should not have separate Dependabot entries. They are already monitored through the parent module's Dependabot configuration entry.
Applied to files:
docs/modules/kafka.md
📚 Learning: 2025-09-29T13:57:14.636Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
Applied to files:
docs/modules/kafka.md
🧬 Code graph analysis (1)
modules/kafka/options_test.go (2)
modules/kafka/kafka.go (1)
Run(55-140)modules/kafka/options.go (1)
WithStarterScript(30-39)
🔇 Additional comments (5)
modules/kafka/version_test.go (2)
9-71: LGTM! Comprehensive test coverage for Apache image detection.The test thoroughly covers both apache/kafka-native and apache/kafka images with various tag formats and the docker.io prefix, plus negative cases. The use of require.Equal is idiomatic and provides clear failure messages.
73-115: LGTM! Solid test coverage for Confluent image detection.The test covers confluentinc/cp-kafka images with various tags and the docker.io prefix, plus negative cases. The structure mirrors Test_isApache and maintains consistency across the test file.
modules/kafka/options_test.go (1)
31-54: LGTM: Comprehensive Confluent and fallback coverage.The test cases for Confluent images and the custom image fallback are well-structured and align with the backward compatibility goal mentioned in the PR objectives.
docs/modules/kafka.md (2)
31-35: Verify benchmark timings and image sizes are accurate and current.The comparison table at lines 31–35 includes specific Docker image sizes and startup timings. Per the PR objectives, these are benchmarked values (e.g., Apache Native ~3.693s, Apache ~5.65s, Confluent ~14.77s in testing). However, Docker image sizes and performance can vary with image tags and environments. Ensure that:
- Sizes and timings match the specific image tags being recommended (e.g., apache/kafka:4.0.1, apache/kafka-native:4.0.1, confluentinc/confluent-local:7.5.0).
- These numbers remain reasonably accurate as images are updated.
- Consider adding a note that timings are representative and may vary by environment.
43-52: Code block references in kafka.md are correct and properly marked in examples_test.go.Verification confirms all three block identifiers referenced in kafka.md (lines 43, 47, 51) are correctly marked in examples_test.go using comment-based markers:
runKafkaContainerApacheNative— marked at line 48runKafkaContainerApacheNotNative— marked at line 81runKafkaContainerConfluentinc— marked at line 15All block names match exactly with no spelling inconsistencies or case sensitivity issues.
4 participants
What does this PR do?
Adding support for
apache/kafkaandapache/kafka-nativeimages into existing kafka module without breaking backward compatibility for users who might have used existing Confluent features.Apache images are significantly faster, especially native one (~2-3 seconds vs 11-17 for example test).
Why is it important?
Put simply Kafka Native is blazingly fast. It is based on GraalVM in native mode, which compiles Java code into native binary. This translates into very fast starting time and starting time is what always important in tests (unlike in runtime) and what is normally very slow for Java. In my relatively simple tests on my relatively powerful machine, the improvement is from 16.6 seconds to 3.6 seconds. Translate this into say 100 tests and the improvement can be measured in minutes if not hours for big projects.
This change also can fix this issue due to changed way of starting the process: #2206
, so long as user would switch image to the one from Apache.
Related issues
How to test this PR
Explain here how this PR will be tested by the reviewer: commands, dependencies, steps, etc.
Check out difference in start times:
Typical outcome:
These are benchmarks that only measure start and stop:
Confluent: ~13.7 s
Apache Native: ~0.9 s
Apache: ~3.7 s
Additional Context
There are two other PR's attempting to add Kafka Native support.
Difference with 2683
A brief difference:
apache/kafkaandapache/kafka-nativeimagesDifference with 3488
A brief difference:
apache/kafka-nativeimage, but also is clear about which module version supports it and about tradeoffs (i.e missing CLI tools)kafka.WithApacheFlavor()orkafka.WithConfluentFlavor()or even pass their own custom script usingkafka.WithStarterScript