Adapt config parsing from flow to exchange #1435
Adapt config parsing from flow to exchange #1435RanMaoyi wants to merge 2 commits intosdv_masterfrom
Conversation
Signed-off-by: Moi Ran <maoyi.ran@emqx.io>
Signed-off-by: Moi Ran <maoyi.ran@emqx.io>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
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 |
There was a problem hiding this comment.
Pull request overview
This pull request implements backward-compatible configuration parsing that supports both "flow" and "exchange" terminology in config files while maintaining internal "exchange" semantics. The PR also includes improvements to encryption key handling in the Parquet module and adds required CURL dependency for Arrow integration.
Changes:
- Added backward-compatible config parsing to support both "flow_" and "exchange_" configuration keys with precedence for "flow_*"
- Enhanced null-safety for encryption/decryption keys in Parquet module by wrapping them in SecureString
- Added CURL dependency in CMakeLists.txt to prevent CMake configuration errors when Arrow references CURL
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/supplemental/nanolib/conf_ver2.c | Implements backward-compatible config parsing that tries "flow_" keys first, then falls back to "exchange_" keys for exchange_client, exchange_url, and exchange objects |
| src/supplemental/nanolib/parquet/parquet.cc | Wraps encryption/decryption keys in SecureString with null checks, removes unnecessary DeepClone() call |
| src/supplemental/nanolib/parquet/CMakeLists.txt | Adds CURL package requirement before Arrow to ensure proper dependency resolution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Compat: allow using "flow" in config file while keeping internal | ||
| // semantics as "exchange". Support both old and new keys: | ||
| // - exchange_client <-> flow_client | ||
| // - exchange <-> flow | ||
| // | ||
| // Precedence: prefer "flow_*" when both exist. |
There was a problem hiding this comment.
The PR title "Adapt config parsing from flow to exchange" suggests migrating from "flow" to "exchange" terminology. However, the code changes implement the opposite: they prefer "flow_" keys over "exchange_" keys (see lines 1765-1768, 1790-1793, 1796-1799), treating "flow" as the new/preferred terminology and "exchange" as the old/backward-compatible option. This discrepancy between the PR title and the actual implementation may cause confusion. Please verify the intended direction of the migration and ensure consistency between the PR description and the code changes.
| # Arrow's exported CMake targets may reference CURL::libcurl in their | ||
| # INTERFACE_LINK_LIBRARIES. Ensure the CURL imported target exists before | ||
| # including ArrowTargets.cmake via find_package(Arrow). | ||
| find_package(CURL REQUIRED) |
No description provided.