cmake: kafka: enable zstd compression using bundled library#11726
cmake: kafka: enable zstd compression using bundled library#11726zshuang0316 wants to merge 1 commit intofluent:masterfrom
Conversation
zshuang0316
requested review from
celalettin1286,
niedbalski and
patrick-stephens
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCMake changes in Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e647c1018
ℹ️ 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".
| set(ZSTD_INCLUDE_DIR "${FLB_PATH_ROOT_SOURCE}/${FLB_PATH_LIB_ZSTD}/lib" CACHE PATH "" FORCE) | ||
| set(ZSTD_LIBRARY_DEBUG libzstd_static CACHE STRING "" FORCE) | ||
| set(ZSTD_LIBRARY_RELEASE libzstd_static CACHE STRING "" FORCE) | ||
| set(ZSTD_LIBRARY libzstd_static CACHE STRING "" FORCE) |
There was a problem hiding this comment.
This hard-codes librdkafka to link libzstd_static even in builds that explicitly prefer system zstd. In the FLB_PREFER_SYSTEM_LIB_ZSTD=ON path, Fluent Bit may skip cmake/zstd.cmake and therefore never create the libzstd_static target (CMakeLists.txt zstd selection block), so this value is no longer a valid target and can become a bare -llibzstd_static link flag instead of using the discovered system zstd library, breaking Kafka-enabled builds in that configuration.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake/kafka.cmake`:
- Line 87: The CMake option FLB_OPTION(WITH_ZSTD ON) currently causes the
summary to always say "ON (bundled)" even when the cached or user value disables
ZSTD; update the logic so the summary reflects the actual WITH_ZSTD value or
force-set WITH_ZSTD explicitly. Locate the FLB_OPTION(WITH_ZSTD ON) call and
change the summary printing code to read the real cached variable (e.g., check
${WITH_ZSTD} or call option/getting function) and print "ON (bundled)" only when
the variable is true, or alternatively set WITH_ZSTD explicitly before the
summary so the hard-coded text is correct; apply the same change to the related
summary block around the other occurrence (the block at the second appearance).
- Around line 80-86: The current cmake snippet force-sets ZSTD_* variables to
the bundled libzstd_static even when that target may not be defined; wrap the
set(...) calls for ZSTD_FOUND, ZSTD_INCLUDE_DIR, ZSTD_LIBRARY_DEBUG,
ZSTD_LIBRARY_RELEASE and ZSTD_LIBRARY in a guard that checks if the CMake target
libzstd_static exists (use if(TARGET libzstd_static) ... endif()), so librdkafka
is only forced to use libzstd_static when that bundled target was actually
created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
zshuang0316
force-pushed
the
feature/kafka-zstd-integration
branch
from
9e647c1 to
aa6e6cb
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa6e6cbaaf
ℹ️ 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".
| # Enable zstd compression for librdkafka | ||
| # Prefer system zstd when FLB_PREFER_SYSTEM_LIB_ZSTD is enabled and found, | ||
| # otherwise fall back to bundled zstd library | ||
| if(FLB_PREFER_SYSTEM_LIB_ZSTD AND ZSTD_FOUND) |
There was a problem hiding this comment.
When -DFLB_PREFER_SYSTEM_LIB_ZSTD=On, the top-level build can detect zstd through pkg-config (CMakeLists.txt sets LIBZSTD_FOUND in the fallback path) while leaving ZSTD_FOUND false; this condition then falls through to the else block and forces WITH_ZSTD off. In that scenario, Kafka zstd compression is disabled even though librdkafka still successfully finds system zstd later, so users on distros without a CMake ZSTD config module lose zstd support unexpectedly.
Useful? React with 👍 / 👎.
zshuang0316
force-pushed
the
feature/kafka-zstd-integration
branch
from
aa6e6cb to
4399ec9
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db316512a8
ℹ️ 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".
| set(ZSTD_INCLUDE_DIR "${FLB_PATH_ROOT_SOURCE}/${FLB_PATH_LIB_ZSTD}/lib" CACHE PATH "" FORCE) | ||
| set(ZSTD_LIBRARY_DEBUG libzstd_static CACHE STRING "" FORCE) | ||
| set(ZSTD_LIBRARY_RELEASE libzstd_static CACHE STRING "" FORCE) | ||
| set(ZSTD_LIBRARY libzstd_static CACHE STRING "" FORCE) |
There was a problem hiding this comment.
Forcing ZSTD_LIBRARY* into the cache here leaks across reconfigures: if a build directory is first configured with bundled zstd and then reconfigured with -DFLB_PREFER_SYSTEM_LIB_ZSTD=On, the system branch never unsets these cache entries, so librdkafka's find_package(ZSTD) reuses libzstd_static from cache instead of the system library. In a reconfigure test, CMake still reported Found ZSTD: libzstd_static while the summary said ZSTD: system, and this can produce link failures (-lzstd_static) on hosts that only provide libzstd.
Useful? React with 👍 / 👎.
zshuang0316
force-pushed
the
feature/kafka-zstd-integration
branch
from
db31651 to
b9c810b
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9c810b556
ℹ️ 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".
| set(ZSTD_LIBRARY_RELEASE libzstd_static CACHE STRING "" FORCE) | ||
| set(ZSTD_LIBRARY libzstd_static CACHE STRING "" FORCE) | ||
| set(FLB_KAFKA_ZSTD_SOURCE "bundled") | ||
| FLB_OPTION(WITH_ZSTD ON) |
There was a problem hiding this comment.
Setting WITH_ZSTD here uses FLB_OPTION, which writes an INTERNAL cache entry, but the system-zstd path does not unset it later. If a build directory is first configured with bundled zstd and then reconfigured to use system zstd where discovery fails, WITH_ZSTD remains ON while the code clears ZSTD_* cache entries, so librdkafka proceeds with zstd enabled but without a resolved library/include path (observed as Found ZSTD: with an empty value), which can break the Kafka build/link step.
Useful? React with 👍 / 👎.
| endif() | ||
| else() | ||
| set(FLB_KAFKA_ZSTD_SOURCE "disabled") | ||
| FLB_OPTION(WITH_ZSTD OFF) |
There was a problem hiding this comment.
This branch force-disables WITH_ZSTD via INTERNAL cache whenever LIBZSTD_LIBRARIES is empty, which overrides explicit caller intent (for example -DWITH_ZSTD=ON plus custom ZSTD_LIBRARY/ZSTD_INCLUDE_DIR in cross or non-standard sysroot builds). That means Kafka zstd support is turned off even when the user has provided valid manual zstd paths that librdkafka could otherwise use.
Useful? React with 👍 / 👎.
Configure librdkafka to use zstd compression. Prefer system zstd when FLB_PREFER_SYSTEM_LIB_ZSTD is enabled and found, otherwise fall back to bundled zstd library. The CACHE FORCE flags ensure the bundled library takes precedence over any librdkafka find_package(ZSTD) results when using bundled. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: zshuang0316 <zshuang0316@163.com>
zshuang0316
force-pushed
the
feature/kafka-zstd-integration
branch
from
b9c810b to
d8ca5de
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8ca5de0c1
ℹ️ 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".
| if(ZSTD_LIBRARY STREQUAL "libzstd_static") | ||
| unset(ZSTD_FOUND CACHE) | ||
| unset(ZSTD_INCLUDE_DIR CACHE) |
There was a problem hiding this comment.
When zstd is discovered through Fluent Bit's pkg-config fallback (CMakeLists.txt zstd block), this system path does not populate ZSTD_INCLUDE_DIR/ZSTD_LIBRARY or force WITH_ZSTD on unless a bundled sentinel is present. In that case, add_subdirectory(librdkafka) runs its own find_package(ZSTD) and may keep WITH_ZSTD=OFF, so Kafka zstd compression is silently disabled despite zstd having been found earlier. Please propagate the resolved system zstd include/library values (or explicitly enable WITH_ZSTD) in this branch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
When zstd is found via pkg-config it is present in standard system paths that librdkafka's find_package(ZSTD) will also search. If the two diverge it indicates a non-standard prefix where the caller should supply CMAKE_PREFIX_PATH or -DZSTD_ROOT; silently forwarding pkg-config results into librdkafka's find variables would reintroduce the stale-cache issues this change was designed to avoid.
1 participant
Configure librdkafka to use Fluent Bit's bundled zstd library for compression support. This avoids external dependency on system zstd while enabling zstd compression in Kafka producers/consumers.
The CACHE FORCE flags ensure the bundled library takes precedence over any system find_package(ZSTD) results.
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.
ok-package-testlabel to test for all targets (requires maintainer to do).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
New Features
Chores