Fix compile for gcs - #247
Conversation
WalkthroughCMakeLists.txt is updated to introduce a new data store option ELOQDSS_ROCKSDB_CLOUD_GCS with KV_STORAGE_VAL set to 3 and compile definition DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_GCS. The RocksDB Cloud library checking logic is consolidated by moving the ROCKSDB_CLOUD_LIB existence check earlier with logging and removing redundant checks later in the configuration. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CMakeLists.txt (2)
363-364: Add ELOQDSS_ROCKSDB_CLOUD_GCS to library linking condition (line 363).The library linking condition excludes ELOQDSS_ROCKSDB_CLOUD_GCS, preventing ROCKSDB_LIBRARIES from being linked for GCS builds. This will cause linker failures.
Line 363 must include ELOQDSS_ROCKSDB_CLOUD_GCS:
-if(WITH_DATA_STORE STREQUAL "ELOQDSS_ROCKSDB" OR WITH_DATA_STORE STREQUAL "ELOQDSS_ROCKSDB_CLOUD_S3") +if(WITH_DATA_STORE STREQUAL "ELOQDSS_ROCKSDB" OR WITH_DATA_STORE STREQUAL "ELOQDSS_ROCKSDB_CLOUD_S3" OR WITH_DATA_STORE STREQUAL "ELOQDSS_ROCKSDB_CLOUD_GCS")
160-209: Add GCS-specific library discovery for ROCKSDB_CLOUD_GCS in root CMakeLists.txt.The root CMakeLists.txt (lines 160–209) includes
ROCKSDB_CLOUD_GCSin the outer condition (line 160), but provides no corresponding GCS-specific library discovery block. Lines 162–199 handle S3-specific AWS SDK dependencies (aws-cpp-sdk-core,aws-cpp-sdk-s3, androcksdb-cloud-aws), but there is no parallel GCS block for Google Cloud dependencies. WhenROCKSDB_CLOUD_GCSis selected, only the common RocksDB Cloud header setup (lines 200–209) executes, leaving Google Cloud SDK headers and libraries undiscovered.Add a GCS-specific section after line 199 (following the S3 block), parallel in structure to lines 162–199. It should:
- Find Google Cloud Storage headers (
google/cloud/storage/client.h)- Find and validate Google Cloud libraries (
google_cloud_cpp_common,google_cloud_cpp_storage)- Find and validate the GCS-specific RocksDB Cloud library (
rocksdb-cloud-gcpor equivalent)- Add these to
ROCKSDB_INCLUDE_PATHandROCKSDB_LIBRARIESNote: The
ELOQDSS_ROCKSDB_CLOUD_GCSvariant is handled separately instore_handler/eloq_data_store_service/CMakeLists.txt(lines 247–264), but the plainROCKSDB_CLOUD_GCSoption requires this addition to the root CMakeLists.txt.
| elseif(WITH_DATA_STORE STREQUAL "ELOQDSS_ROCKSDB_CLOUD_GCS") | ||
| set(KV_STORAGE_VAL 3 CACHE STRING "eloq_ds" FORCE) | ||
| add_compile_definitions(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_GCS) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get line count to understand file size
wc -l CMakeLists.txt
# Check lines around 299 (source files condition)
echo "=== Lines 295-305 (source files condition) ==="
sed -n '295,305p' CMakeLists.txt
# Check lines around 363 (library linking condition)
echo "=== Lines 360-370 (library linking condition) ==="
sed -n '360,370p' CMakeLists.txt
# Check lines around 213 (proto compilation)
echo "=== Lines 210-220 (proto compilation) ==="
sed -n '210,220p' CMakeLists.txt
# Search for all GCS-related configuration
echo "=== All GCS references in file ==="
grep -n "GCS\|gcs" CMakeLists.txt
# Check library discovery patterns (S3 section)
echo "=== Lines 162-199 (S3 library discovery section) ==="
sed -n '162,199p' CMakeLists.txtRepository: eloqdata/tx_service
Length of output: 5748
🏁 Script executed:
#!/bin/bash
# Check S3-specific source files (lines 320-326 mentioned in review)
echo "=== Lines 315-330 (S3-specific sources) ==="
sed -n '315,330p' CMakeLists.txt
# Search for any GCS-specific source files or patterns
echo "=== Search for GCS source file references ==="
rg -n "gcs|GCS" CMakeLists.txt | grep -i "source\|file\|\.cpp"
# Check what source files are in the data substrate sources block
echo "=== Lines 299-350 (full source files block) ==="
sed -n '299,350p' CMakeLists.txt
# Look for rocksdb-cloud library naming
echo "=== Lines 190-200 (rocksdb-cloud library pattern) ==="
sed -n '190,200p' CMakeLists.txtRepository: eloqdata/tx_service
Length of output: 4364
🏁 Script executed:
#!/bin/bash
# Search for gcs file downloader or GCS-specific sources
echo "=== Search for GCS source patterns ==="
find . -name "*.cpp" -o -name "*.h" | xargs grep -l "gcs\|GCS\|google.*cloud" 2>/dev/null | head -20
# Check for rocksdb-cloud-gcs references
echo "=== Search for rocksdb-cloud-gcs ==="
rg "rocksdb-cloud-gcs" --type cmake --type cpp --type h
# List available source files related to cloud storage
echo "=== Available cloud storage related sources ==="
find store_handler -name "*downloader*" -o -name "*cloud*" 2>/dev/null
# Check if gcs_file_downloader exists or similar
echo "=== Check for any GCS-related implementation files ==="
find . -type f \( -name "*.cpp" -o -name "*.h" \) | xargs basename -a | grep -i gcsRepository: eloqdata/tx_service
Length of output: 1236
🏁 Script executed:
#!/bin/bash
# Check rocksdb_cloud_data_store.cpp for GCS support
echo "=== rocksdb_cloud_data_store.cpp content (first 100 lines) ==="
head -100 store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp
# Check for conditional compilation markers or cloud provider handling
echo "=== Search for cloud provider logic in rocksdb_cloud_data_store ==="
rg -n "S3|GCS|google|aws" store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cppRepository: eloqdata/tx_service
Length of output: 4282
Perfect. I've now gathered enough evidence to verify the review comment. The verification shows that:
- Line 299 condition is indeed incomplete - it excludes
ELOQDSS_ROCKSDB_CLOUD_GCSfrom source file inclusion - Line 363 condition is indeed incomplete - it excludes
ELOQDSS_ROCKSDB_CLOUD_GCSfrom library linking - GCS library discovery block is missing - only S3 has library discovery (rocksdb-cloud-aws)
- GCS source handling pattern is missing - no equivalent to the S3 elseif block (lines 320-326)
Most critically, rocksdb_cloud_data_store.cpp already contains GCS support (with #ifdef DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_GCS), but CMakeLists.txt prevents it from being compiled for GCS builds.
Add ELOQDSS_ROCKSDB_CLOUD_GCS to source files, library linking, and library discovery conditions.
The new ELOQDSS_ROCKSDB_CLOUD_GCS option is partially integrated: compile definitions are set (line 128), and rocksdb_cloud_data_store.cpp contains GCS-specific code with #ifdef DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_GCS. However, three critical gaps prevent GCS builds from compiling:
- Line 299: Add
ELOQDSS_ROCKSDB_CLOUD_GCSto the source file inclusion condition alongsideELOQDSS_ROCKSDB_CLOUD_S3 - Lines 320–326: Add an
elseif(WITH_DATA_STORE STREQUAL "ELOQDSS_ROCKSDB_CLOUD_GCS")block for GCS-specific sources (e.g.,gcs_file_downloader.cppor platform-specific equivalent) - Line 363: Add
ELOQDSS_ROCKSDB_CLOUD_GCSto the library linking condition - Lines 162–199: Add a GCS-specific library discovery block (similar to S3) to find
rocksdb-cloud-gcslibrary and Google Cloud C++ headers
🤖 Prompt for AI Agents
In CMakeLists.txt around lines 126-199, 299, 320-326, and 363, the new
DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_GCS compile definition is set but CMake
does not include GCS sources, discover GCS libraries/headers, or link GCS
libraries; update the file by (1) adding ELOQDSS_ROCKSDB_CLOUD_GCS to the
source-file inclusion condition at ~line 299 alongside ELOQDSS_ROCKSDB_CLOUD_S3,
(2) inserting an elseif(WITH_DATA_STORE STREQUAL "ELOQDSS_ROCKSDB_CLOUD_GCS")
block in the sources section (around lines 320–326) mirroring the S3 block and
listing GCS-specific sources (e.g., gcs downloader/adapter files), (3) adding
ELOQDSS_ROCKSDB_CLOUD_GCS to the library-linking condition at ~line 363 so the
rocksdb-cloud-gcs lib is linked, and (4) adding a GCS library discovery block
(modeled on the existing S3 discovery in lines ~162–199) to find the
rocksdb-cloud-gcs library and Google Cloud C++ headers and set the appropriate
include/link variables.
2 participants
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.