adapt to new parameters and add new dependencies#268
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds AWS S3 integration by linking AWSSDK into the build and introduces six new EloqStore cloud configuration flags and corresponding config fields while removing legacy cloud_store_daemon_ports handling. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
store_handler/eloq_data_store_service/build_eloq_store.cmake (1)
115-115: Linking with${AWSSDK_LINK_LIBRARIES}is fine; consider imported targets as a future cleanupAppending
${AWSSDK_LINK_LIBRARIES}totarget_link_libraries(eloqstore PRIVATE ...)is a reasonable way to pull in S3 and its transitive dependencies, and keeping itPRIVATEis appropriate for a static library. Two follow‑ups to keep in mind:
- Ensure your AWSSDK CMake package actually defines
AWSSDK_LINK_LIBRARIES; some installs rely solely on imported targets likeAWS::aws-cpp-sdk-s3.- Longer term, consider switching to explicit imported targets for clarity and stronger CMake hygiene, e.g. (names may differ depending on your SDK version):
-target_link_libraries(eloqstore PRIVATE ${URING_LIB} Boost::context glog::glog absl::flat_hash_map jsoncpp_lib ${CURL_LIBRARIES} ${ZSTD_LIBRARY} ${AWSSDK_LINK_LIBRARIES}) +target_link_libraries(eloqstore PRIVATE + ${URING_LIB} + Boost::context + glog::glog + absl::flat_hash_map + jsoncpp_lib + ${CURL_LIBRARIES} + ${ZSTD_LIBRARY} + AWS::aws-cpp-sdk-s3 # or the appropriate imported target(s) for your AWSSDK version +)This can be deferred; current linking is acceptable if it works with your installed SDK.
store_handler/eloq_data_store_service/eloq_store_config.cpp (1)
287-322: Config wiring for new cloud fields looks correct; consider small usability checks.The new
cloud_*fields follow the existing pattern (CheckCommandLineFlagIsDefaultwith INI fallback) and the types (string vs bool) line up correctly with theGetString/GetBooleancalls, so behavior here looks sound.As an optional follow‑up, you might:
- Validate combinations early (e.g., non‑empty
cloud_store_pathwith missingcloud_regionor credentials, depending on provider), and- Log non‑sensitive pieces like
cloud_provider,cloud_region,cloud_endpoint, andcloud_verify_sslat startup (keeping keys out of logs) to simplify debugging misconfigurations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
store_handler/eloq_data_store_service/build_eloq_store.cmake(2 hunks)store_handler/eloq_data_store_service/eloq_store_config.cpp(2 hunks)
🔇 Additional comments (1)
store_handler/eloq_data_store_service/build_eloq_store.cmake (1)
29-29: Confirm AWSSDK CMake package name and S3 component match your SDK installation
find_package(AWSSDK REQUIRED COMPONENTS s3)is consistent with the official AWS SDK for C++ CMake integration, but some environments/distros expose the SDK under different package names or component sets (e.g.,AwsSdkCpp, or needing additional components likecore). Please verify thisfind_packagecall against the actual AWS SDK CMake config used in your CI/build images so configuration doesn’t fail at configure time.
Here are some reminders before you submit the pull request
fixes eloqdb/tx_service#issue_id./mtr --suite=mono_main,mono_multi,mono_basicSummary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.