Repository navigation
Conversation
The container builder stages did not install libzstd-dev, so librdkafka's find_package(ZSTD) failed and WITH_ZSTD was disabled. Kafka producers in the resulting images could not use zstd compression. Add libzstd-dev to the builder and debug builder stages so librdkafka is compiled with zstd support. Signed-off-by: Domenic Bottini <dbottini@atlassian.com>
Add a runtime test that sets compression.codec=zstd on a librdkafka conf. rd_kafka_conf_set only accepts the zstd codec when librdkafka was built with WITH_ZSTD, so this guards against images shipping without zstd support. Signed-off-by: Domenic Bottini <dbottini@atlassian.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the
Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99acbc8514
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| res = rd_kafka_conf_set(conf, "compression.codec", "zstd", | ||
| errstr, sizeof(errstr)); | ||
| TEST_CHECK(res == RD_KAFKA_CONF_OK); |
There was a problem hiding this comment.
Gate the zstd check to zstd-enabled builds
In regular source builds where FLB_OUT_KAFKA=On but the host lacks a system libzstd-dev, librdkafka still builds with WITH_ZSTD=OFF because the CMake path only lets librdkafka's own find_package(ZSTD) decide; the Dockerfile install does not affect those builds. This unconditional assertion then makes flb-rt-out_kafka fail for an otherwise supported non-container build, so either wire zstd into the CMake Kafka build or only run this check when zstd support was actually required/enabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in
0c6dde112. Zstd assertion now runs only for bundled librdkafka withWITH_ZSTD=ON; optional/system builds retainraw_format. Kafka runtime tests pass with zstd ON and OFF.
Rovo Dev
The out_kafka runtime test includes librdkafka's public rdkafka.h header directly, but the generic runtime test build loop does not add the kafka include path the in_kafka/out_kafka plugins rely on. This breaks the build with 'rdkafka.h: No such file or directory'. Add the librdkafka include directory to the flb-rt-out_kafka target, covering both the system librdkafka (pkg-config) and bundled librdkafka builds. Co-authored-by: Rovo Dev (Claude) Signed-off-by: Domenic Bottini <dbottini@atlassian.com>
Co-authored-by: Rovo Dev (Claude) Signed-off-by: Domenic Bottini <dbottini@atlassian.com>
Only assert zstd availability for bundled librdkafka builds with WITH_ZSTD enabled. System and bundled no-zstd builds retain the raw-format runtime test. Co-authored-by: Rovo Dev (Claude) Signed-off-by: Domenic Bottini <dbottini@atlassian.com>
Co-authored-by: Rovo Dev (Claude) Signed-off-by: Domenic Bottini <dbottini@atlassian.com>
Co-authored-by: Rovo Dev (Claude) Signed-off-by: Domenic Bottini <dbottini@atlassian.com>
Container images were built without
libzstd-devin the builder stage, solibrdkafka's
find_package(ZSTD)failed andWITH_ZSTDwas disabled. Kafkaproducers in the shipped images could not use zstd compression.
Changes:
dockerfiles/Dockerfile: addlibzstd-devto the builder and debug builderstages so librdkafka is compiled with zstd support.
tests/runtime/out_kafka.c: add a runtime test asserting librdkafka acceptscompression.codec=zstd(only true whenWITH_ZSTDis compiled in).Addresses #11366 (Packaging: Enable ZSTD compression by default).
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
local-build-all.shbuilds the native distro packages underpackaging/distros/. This PR only changes the container images (dockerfiles/Dockerfile), which that script does not build. The container image build was verified instead (see test evidence above).ok-package-testlabel to test for all targets (requires maintainer to do).Test evidence
Example config (
rdkafka.compression.codec=zstd):Container build (
dockerfiles/Dockerfile,--target=production) — zstd is nowdetected so librdkafka is built with
WITH_ZSTD:Running the production image with the kafka zstd config — the output starts and
accepts the codec (only failure is the absent broker, as expected). Before this change
the same image rejected
compression.codec=zstdat init:Runtime test (
tests/runtime/out_kafka.c::zstd_compression_available) under Valgrind:Notes on targets: amd64 and arm64 (the published distroless images) configure with
zstd and build successfully. The README's plain multi-arch example additionally lists
linux/arm/v7andlinux/s390x, which fail atCMakeLists.txt:310because-DFLB_SIMD=Onis unsupported on those architectures; this is pre-existing andunrelated to this change.
Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
Chores
Tests