Replace USE_ROCKSDB_LOG_STATE and WITH_ROCKSDB_CLOUD compile flags#224
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughCentralizes log-state selection under a new WITH_LOG_STATE enum (MEMORY, ROCKSDB, ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS); adds GCS cloud backend; updates build scripts, docs, CMake/SCons discovery, and engine sources to use WITH_LOG_STATE and LOG_STATE_TYPE_*; bumps several eloq submodule pointers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant BS as Build Scripts
participant CMake as CMake (eloq)
participant Find as find_dependencies.cmake
participant AWS as AWS SDK
participant GCP as GCP SDK
participant RKDB as RocksDB / RocksDB-Cloud
Dev->>BS: set WITH_LOG_STATE & WITH_DATA_STORE
BS->>CMake: cmake -DWITH_LOG_STATE=... -DWITH_DATA_STORE=...
CMake->>Find: query dependencies for WITH_LOG_STATE
alt WITH_LOG_STATE = MEMORY
Find-->>CMake: skip RocksDB (no includes/libs)
else WITH_LOG_STATE = ROCKSDB
Find->>RKDB: locate base RocksDB
RKDB-->>Find: include/lib paths
else WITH_LOG_STATE = ROCKSDB_CLOUD_S3
Find->>AWS: locate aws-cpp-sdk (core/s3/kinesis)
AWS-->>Find: include/lib paths
Find->>RKDB: locate rocksdb-cloud-aws
RKDB-->>Find: include/lib paths
else WITH_LOG_STATE = ROCKSDB_CLOUD_GCS
Find->>GCP: locate google cloud storage libs
GCP-->>Find: include/lib paths
Find->>RKDB: locate rocksdb-cloud-gcp
RKDB-->>Find: include/lib paths
end
CMake-->>Dev: configure targets with LOG_STATE_TYPE_*
sequenceDiagram
autonumber
participant Server as Mongo Server
participant KV as Eloq KV Engine
participant RKDB as RocksDB / RocksDB-Cloud
participant AWS as AWS SDK (optional)
participant GCP as GCP SDK (optional)
Server->>KV: Initialize (LOG_STATE_TYPE_*)
alt LOG_STATE_TYPE_RKDB
KV->>RKDB: open local RocksDB
else LOG_STATE_TYPE_RKDB_S3
KV->>AWS: awsInit()
KV->>RKDB: open rocksdb-cloud-aws
else LOG_STATE_TYPE_RKDB_GCS
KV->>GCP: init GCS client
KV->>RKDB: open rocksdb-cloud-gcp
else LOG_STATE_TYPE_MEM
KV-->>Server: use in-memory log state
end
Server-->>KV: Shutdown
opt Cloud backend
alt S3
KV->>AWS: aws_deinit()
else GCS
KV->>GCP: teardown
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (14)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/mongo/db/modules/eloq/cmake/build_log_service.cmake (1)
140-140: Move Protobuf discovery before its usage.The
find_package(Protobuf REQUIRED)call at line 140 occurs after${PROTOBUF_LIBRARY}is already referenced in theLOG_LIBvariable at line 131. This means the variable is used before it's defined by the package discovery.Apply this fix by moving the Protobuf discovery earlier in the file:
include_directories(${ROCKSDB_INCLUDE_PATH}) +find_package(Protobuf REQUIRED) + set(LOG_LIB ${LOG_LIB} ${ROCKSDB_LIB} @@ -123,6 +125,0 @@ ${TX_LOG_PROTOS_SOURCE_DIR} ) -find_package(Protobuf REQUIRED) -The Protobuf discovery should occur before line 111 where
LOG_LIBis defined, ensuring${PROTOBUF_LIBRARY}is available when referenced at line 131.src/mongo/db/modules/eloq/cmake/find_dependencies.cmake (2)
267-267: Fix typo in status message."requied" should be "required".
Apply this diff:
- message(STATUS "Dependencies: RocksDB is requied") + message(STATUS "Dependencies: RocksDB is required")
273-273: Fix typo in status message."Dependenies" should be "Dependencies" to match the consistent prefix used throughout the file.
Apply this diff:
- message(STATUS "Dependenies: RocksDB lib path ${ROCKSDB_LIB}") + message(STATUS "Dependencies: RocksDB lib path ${ROCKSDB_LIB}")
🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/SConscript (1)
103-111: Consider adding a default case or validation for WITH_LOG_STATE.The new log state configuration correctly maps environment variable values to compile-time defines. However, if
WITH_LOG_STATEis unset or contains an invalid value, no defines are added and the build proceeds silently. This could lead to inconsistent behavior.Consider adding a warning message (similar to lines 75-77 for
WITH_DATA_STORE) or a default case to make the build more robust and easier to debug.Apply this diff to add validation:
with_log_state = os.environ.get("WITH_LOG_STATE", "").upper() if with_log_state == "MEMORY": env.Append(CPPDEFINES=["LOG_STATE_TYPE_MEM"]) elif with_log_state == "ROCKSDB": env.Append(CPPDEFINES=["LOG_STATE_TYPE_RKDB"]) elif with_log_state == "ROCKSDB_CLOUD_S3": env.Append(CPPDEFINES=["LOG_STATE_TYPE_RKDB_S3"]) elif with_log_state == "ROCKSDB_CLOUD_GCS": env.Append(CPPDEFINES=["LOG_STATE_TYPE_RKDB_GCS"]) +elif with_log_state != "": + print("Warning: Unknown WITH_LOG_STATE '{}'. No log state defines set.".format(with_log_state))src/mongo/db/modules/eloq/cmake/find_dependencies.cmake (1)
187-194: Relocate or remove the misplaced comment on line 190.The comment "Check for GCS cloud requirement from log_service configuration" appears between the S3 conditional block and the GCS elseif clause, making it unclear what it documents.
Apply this diff to remove the redundant comment:
if(DEFINED WITH_LOG_STATE) if(WITH_LOG_STATE STREQUAL "ROCKSDB_CLOUD_S3") set(NEED_ROCKSDB_CLOUD_S3 ON) - # Check for GCS cloud requirement from log_service configuration elseif(WITH_LOG_STATE STREQUAL "ROCKSDB_CLOUD_GCS") set(NEED_ROCKSDB_CLOUD_GCS ON) endif() endif()Alternatively, move it before line 187 as a general comment documenting both branches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
concourse/scripts/build_tarball.bash(2 hunks)concourse/scripts/build_tarball_open.bash(5 hunks)concourse/scripts/common.sh(4 hunks)docs/how-to-compile.md(2 hunks)src/mongo/db/modules/eloq/CMakeLists.txt(0 hunks)src/mongo/db/modules/eloq/SConscript(5 hunks)src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake(1 hunks)src/mongo/db/modules/eloq/cmake/build_log_service.cmake(1 hunks)src/mongo/db/modules/eloq/cmake/find_dependencies.cmake(3 hunks)src/mongo/db/modules/eloq/log_service(1 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(8 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.h(1 hunks)src/mongo/db/modules/eloq/store_handler(1 hunks)src/mongo/db/modules/eloq/tx_service(1 hunks)
💤 Files with no reviewable changes (1)
- src/mongo/db/modules/eloq/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.
📚 Learning: 2025-09-25T11:58:50.446Z
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.
Applied to files:
concourse/scripts/build_tarball.bashdocs/how-to-compile.mdconcourse/scripts/build_tarball_open.bashsrc/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmakesrc/mongo/db/modules/eloq/src/eloq_kv_engine.cppsrc/mongo/db/modules/eloq/cmake/find_dependencies.cmakesrc/mongo/db/modules/eloq/cmake/build_log_service.cmakeconcourse/scripts/common.sh
📚 Learning: 2025-09-25T12:24:06.434Z
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Applied to files:
src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmakesrc/mongo/db/modules/eloq/cmake/find_dependencies.cmakesrc/mongo/db/modules/eloq/cmake/build_log_service.cmake
📚 Learning: 2025-09-25T12:09:01.276Z
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:95-115
Timestamp: 2025-09-25T12:09:01.276Z
Learning: The log states (LOG_STATE_TYPE_RKDB, LOG_STATE_TYPE_RKDB_S3, LOG_STATE_TYPE_RKDB_GCS) in the EloqDoc codebase are mutually exclusive - only one can be active at a time. The LOG_STATE_TYPE_RKDB_CLOUD macro is a helper that should only be defined for cloud variants (S3/GCS) and not when regular RKDB is active.
Applied to files:
src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmakesrc/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
🔇 Additional comments (29)
src/mongo/db/modules/eloq/tx_service (1)
1-1: Verify tx_service submodule update compatibility
The pointer update fromaf75de6e5d0d068fa9dda2a21b6cdf621ecd1bd0to18d6adbd3cc08801e20057f7c8e521a5728d8d75must be reviewed against the newWITH_LOG_STATEflag refactoring. Insrc/mongo/db/modules/eloq/tx_service, fetch the missing refs and run:git fetch --unshallow git log --oneline af75de6e5d0d068fa9dda2a21b6cdf621ecd1bd0..18d6adbd3cc08801e20057f7c8e521a5728d8d75Confirm there are no incompatibilities with the parent repository’s updated build configuration.
concourse/scripts/build_tarball_open.bash (1)
152-158: LGTM! Flag rename correctly maps DATA_STORE_TYPE to WITH_LOG_STATE.The migration from
-DWITH_ROCKSDB_CLOUDto-DWITH_LOG_STATEis implemented correctly, with proper branching for S3, GCS, and base RocksDB configurations.src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake (4)
9-24: Well-structured state configuration with proper error handling.The new
WITH_LOG_STATEoption cleanly replaces the previous flag system. The cache variable setup withSTRINGSproperty provides IDE/ccmake dropdown support, and the compile definitions correctly map each state to its correspondingLOG_STATE_TYPE_*macro.
27-104: RocksDB discovery logic is correct and handles all states properly.The control flow correctly handles all four states:
- MEMORY: Skips RocksDB discovery (lines 102-104)
- ROCKSDB: Discovers base RocksDB (lines 81-99, else branch)
- ROCKSDB_CLOUD_S3: Discovers AWS SDK + RocksDB Cloud AWS (lines 31-56)
- ROCKSDB_CLOUD_GCS: Discovers GCP SDK + RocksDB Cloud GCP (lines 57-79)
The regex match on line 27 appropriately groups all RocksDB variants together, and the nested conditionals correctly differentiate between cloud and base variants.
Based on learnings: This addresses the previous MEMORY state control flow issue from PR #211 where MEMORY builds incorrectly triggered RocksDB discovery.
70-71: Verify GCP SDK installs exist for every supported environmentThe Ubuntu 24.04 install script includes both google-cloud-cpp and rocksdb-cloud-gcp. Ensure that for each supported platform (e.g., CentOS, macOS) and in README/docs, there are equivalent install steps for the GCP SDK components and the RocksDB GCP library.
47-48: No action needed: installation script covers AWS Core, S3, Kinesis, and rocksdb-cloud-aws
scripts/install_dependency_ubuntu2404.sh clones aws-sdk-cpp@v1.11.521, builds with -DBUILD_ONLY="dynamodb;sqs;s3;kinesis;kafka;transfer" (core is pulled in as dependency) and installs headers/libs to /usr/include and /usr/lib. The RocksDB-Cloud section installs librocksdb-cloud-aws.src/mongo/db/modules/eloq/log_service (1)
1-1: Manual verification required: log_service submodule updateThis submodule pointer bump to commit 4a208682f4f9dfa3986985a96a5d885a8dc3b738 could not be inspected locally. Please manually confirm:
- The commit exists in the log_service repo and has been reviewed for correctness.
- It’s compatible with the new WITH_LOG_STATE configuration (MEMORY, ROCKSDB, ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS).
- It has been tested and passed CI against all supported log state backends.
src/mongo/db/modules/eloq/store_handler (1)
1-1: Verify submodule changes align with the log-state refactor.The submodule pointer updates from
b3e8d0eto35ddc69. Please review the diff between these commits insrc/mongo/db/modules/eloq/store_handlerto confirm the replacement ofUSE_ROCKSDB_LOG_STATE/ROCKSDB_CLOUDwithWITH_LOG_STATEand proper GCS backend integration.concourse/scripts/build_tarball.bash (2)
186-193: LGTM! Correct alignment of DATA_STORE_TYPE to WITH_LOG_STATE.The mapping logic correctly translates the validated DATA_STORE_TYPE values to their corresponding WITH_LOG_STATE configuration. The fallback to ROCKSDB for ELOQDSS_ROCKSDB is appropriate given the earlier validation at lines 98-113.
209-209: LGTM! CMAKE flag correctly updated.The replacement of
-DWITH_ROCKSDB_CLOUDwith-DWITH_LOG_STATE=$WITH_LOG_STATEaligns with the PR objective and uses the environment variable properly configured in lines 186-193.docs/how-to-compile.md (2)
90-90: LGTM! Documentation correctly updated for CMAKE flag.The replacement of the CMAKE flag with
-DWITH_LOG_STATE=ROCKSDB_CLOUD_S3is consistent with the implementation changes and aligns with the PR objective to centralize RocksDB Cloud configuration.
107-107: LGTM! Documentation correctly updated for environment variables.The replacement with
WITH_LOG_STATE=ROCKSDB_CLOUD_S3alongsideWITH_DATA_STORE=ELOQDSS_ROCKSDB_CLOUD_S3is consistent with the implementation and provides clear guidance for users building EloqDoc-RocksDBCloud.src/mongo/db/modules/eloq/cmake/build_log_service.cmake (2)
97-114: LGTM! RocksDB discovery correctly implemented.The unconditional RocksDB discovery is appropriate for the "open log" functionality, which requires RocksDB. The error handling is clear, and the include paths and libraries are properly configured.
116-118: LGTM! Compile definition correctly replaces USE_ROCKSDB_LOG_STATE.The
LOG_STATE_TYPE_RKDBdefinition is appropriate for the "open log" functionality, which exclusively supports the ROCKSDB log state type. This aligns with the PR objective to replace the previous flag naming scheme.Based on learnings.
concourse/scripts/common.sh (3)
68-68: LGTM! Data store configuration updated correctly.The removal of the explicit
ROCKSDB_CLOUD=OFFflag and retention of theWITH_DATA_STOREflag aligns with the new configuration scheme. The absence ofWITH_LOG_STATEhere is appropriate since this build usesOPEN_LOG_SERVICE=ON(line 78), which according to the learnings only supports the standard ROCKSDB log state type.Based on learnings
115-116: LGTM! Log state and data store flags configured correctly.The new
WITH_LOG_STATE=ROCKSDB_CLOUD_S3andWITH_DATA_STORE=ELOQDSS_ROCKSDB_CLOUD_S3flags correctly replace the previousROCKSDB_CLOUDconfiguration approach. Both flags are consistently applied to CMake (lines 115-116) and SCons (line 128) builds.
128-128: LGTM! Environment variables aligned with CMake configuration.The SCons build environment variables correctly mirror the CMake configuration flags from lines 115-116, ensuring consistency across the build toolchain.
src/mongo/db/modules/eloq/SConscript (3)
68-69: LGTM! GCS data store type added correctly.The new
ELOQDSS_ROCKSDB_CLOUD_GCSvariant is correctly mapped toDATA_STORE_TYPE_ROCKSDB_CLOUD, parallel to the S3 variant handling.
80-87: LGTM! Cloud backend discriminated via compile-time defines.The conditional CPPDEFINES correctly distinguish between GCS and S3 backends, allowing the C++ code to conditionally compile based on the selected cloud provider.
155-170: LGTM! Include paths correctly separated by cloud backend.The conditional include logic cleanly separates common RocksDB Cloud headers from provider-specific headers (AWS for S3, Google Cloud for GCS), avoiding unnecessary dependencies and improving build hygiene.
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (7)
65-68: LGTM! Cloud data store helper macro defined correctly.The
ELOQDS_RKDB_CLOUDmacro correctly aggregates both S3 and GCS data store types, simplifying conditional compilation throughout the file.
90-96: LGTM! AWS includes extended to cover log state S3 usage.The condition correctly includes
LOG_STATE_TYPE_RKDB_S3to ensure AWS SDK headers are available when the log state uses S3, independent of the data store backend selection. This properly reflects the separation of log state and data store configuration.
98-122: LGTM! Log state helper macros defined correctly.The helper macros correctly aggregate log state types:
LOG_STATE_TYPE_RKDB_CLOUD: Cloud-only variants (S3/GCS), excluding regular RKDBLOG_STATE_TYPE_RKDB_ALL: All RKDB-based log states (cloud + regular)The logic on lines 102-103 correctly excludes
LOG_STATE_TYPE_RKDBfrom theCLOUDmacro, ensuring mutual exclusivity between regular RKDB and cloud variants, which aligns with the established architecture.Based on learnings
119-122: LGTM! Cloud config header conditionally included.The conditional inclusion of
rocksdb_cloud_config.his appropriate for cloud log state variants and correctly uses theeloq_log_servicepath rather thanlog_service.Based on learnings
274-277: LGTM! AWS initialization condition updated consistently.The
awsInit()condition correctly mirrors the include condition from line 90, ensuring AWS SDK is initialized when either the data store or log state uses S3.
404-478: LGTM! Log server initialization correctly structured for all log state variants.The nested conditional compilation correctly handles:
- All RKDB-based log states (
LOG_STATE_TYPE_RKDB_ALL)- Cloud-specific configuration for S3/GCS (
LOG_STATE_TYPE_RKDB_CLOUD)- S3-specific AWS credentials (
LOG_STATE_TYPE_RKDB_S3)- Regular RKDB as the fallback
The hierarchy is logical and ensures each variant gets the appropriate configuration while sharing common code where applicable.
1232-1235: LGTM! AWS deinitialization condition consistent with initialization.The
aws_deinit()condition correctly mirrors theawsInit()condition from line 274, ensuring proper cleanup of AWS SDK resources.src/mongo/db/modules/eloq/cmake/find_dependencies.cmake (1)
163-166: LGTM! Clean gating logic for WITH_LOG_STATE.The conditional correctly enables RocksDB for all three variants (ROCKSDB, ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) using regex matching.
src/mongo/db/modules/eloq/src/eloq_kv_engine.h (1)
30-35: Remove or relocate unusedlog_utils.hinclude
No symbols fromlog_utils.hare referenced in this header (nor in eloq_kv_engine.cpp); if it’s only needed in the implementation, move the include to eloq_kv_engine.cpp, otherwise remove it.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/mongo/db/modules/eloq/SConscript (1)
244-249: Library linking logic is correct; indentation issue already flagged.The conditional linking correctly separates S3 and GCS library dependencies ensuring the appropriate SDK libraries are linked for each cloud backend.
Note: The indentation inconsistency on line 249 has already been flagged in a previous review.
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/SConscript (1)
155-170: LGTM: Clean conditional include path handling.The refactored include path logic correctly separates cloud-common RocksDB headers from backend-specific SDK headers (AWS for S3, GCS for GCS), and properly filters out None values.
However, lines 143-150 appear to contain legacy include path discovery that overlaps with this new conditional logic. Consider removing the now-redundant
FindIncludePathcalls from the baseCPPPATHextension to avoid confusion:- FindIncludePath("aws/core"), - FindIncludePath("aws/kinesis"), - FindIncludePath("aws/s3"), - # FindIncludePath("google/cloud/storage"), - FindIncludePath("rocksdb_cloud_header/"), - FindIncludePath("rocksdb_cloud_header/rocksdb"), - FindIncludePath("rocksdb_cloud_header/rocksdb/cloud"),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/SConscript(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:95-115
Timestamp: 2025-09-25T12:09:01.276Z
Learning: The log states (LOG_STATE_TYPE_RKDB, LOG_STATE_TYPE_RKDB_S3, LOG_STATE_TYPE_RKDB_GCS) in the EloqDoc codebase are mutually exclusive - only one can be active at a time. The LOG_STATE_TYPE_RKDB_CLOUD macro is a helper that should only be defined for cloud variants (S3/GCS) and not when regular RKDB is active.
🔇 Additional comments (2)
src/mongo/db/modules/eloq/SConscript (2)
68-69: LGTM: GCS cloud variant mapping is consistent.The mapping of
ELOQDSS_ROCKSDB_CLOUD_GCStoDATA_STORE_TYPE_ROCKSDB_CLOUDmirrors the S3 variant and allows later code to distinguish between backends usingdata_store_type_str.
80-87: LGTM: Cloud backend defines are mutually exclusive.The conditional logic correctly ensures only one of
DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_GCSorDATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3is defined, with S3 as the default fallback. This aligns with the mutual exclusivity requirement.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/mongo/db/modules/eloq/SConscript (1)
103-116: Past review comment not fully addressed: Add validation and error on invalid WITH_LOG_STATE.The previous review requested that this code default
with_log_stateto"ROCKSDB"when empty and callsys.exit(1)on unrecognized values to match CMake behavior. Currently, the code only prints a warning and continues with the ROCKSDB default, which allows builds to proceed with potentially incorrect configurations.Apply this diff to add proper default and validation:
-with_log_state = os.environ.get("WITH_LOG_STATE", "").upper() +with_log_state = os.environ.get("WITH_LOG_STATE", "ROCKSDB").upper() if with_log_state == "MEMORY": env.Append(CPPDEFINES=["LOG_STATE_TYPE_MEM"]) elif with_log_state == "ROCKSDB": env.Append(CPPDEFINES=["LOG_STATE_TYPE_RKDB"]) elif with_log_state == "ROCKSDB_CLOUD_S3": env.Append(CPPDEFINES=["LOG_STATE_TYPE_RKDB_S3"]) elif with_log_state == "ROCKSDB_CLOUD_GCS": env.Append(CPPDEFINES=["LOG_STATE_TYPE_RKDB_GCS"]) -else +else: - # Default fallback (or raise an error if invalid) - print("Warning: Unknown WITH_LOG_STATE '{}'. Defaulting to ROCKSDB.".format(with_log_state)) - env.Append(CPPDEFINES=["LOG_STATE_TYPE_RKDB"]) + print("Error: Unknown WITH_LOG_STATE '{}'. Valid values: MEMORY, ROCKSDB, ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS".format(with_log_state)) + sys.exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/SConscript(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:95-115
Timestamp: 2025-09-25T12:09:01.276Z
Learning: The log states (LOG_STATE_TYPE_RKDB, LOG_STATE_TYPE_RKDB_S3, LOG_STATE_TYPE_RKDB_GCS) in the EloqDoc codebase are mutually exclusive - only one can be active at a time. The LOG_STATE_TYPE_RKDB_CLOUD macro is a helper that should only be defined for cloud variants (S3/GCS) and not when regular RKDB is active.
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
🔇 Additional comments (4)
src/mongo/db/modules/eloq/SConscript (4)
68-69: LGTM!The GCS data store mapping is consistent with the existing S3 pattern, both correctly mapping to
DATA_STORE_TYPE_ROCKSDB_CLOUDwith differentiation handled downstream.
80-87: LGTM!The conditional CPPDEFINES correctly distinguish between GCS and S3 cloud backends, with S3 as the explicit default for ROCKSDB_CLOUD builds.
159-174: LGTM!The conditional include path configuration correctly distinguishes base RocksDB Cloud headers from backend-specific SDK headers (AWS vs GCS), with proper None-filtering for missing paths.
248-253: LGTM!The conditional linking logic correctly separates S3 and GCS library dependencies. The AWS SDK libraries are linked for S3 builds (required for Aws::InitAPI/Aws::ShutdownAPI calls), and Google Cloud C++ libraries are linked for GCS builds.
Note: The indentation issue flagged in a previous review (line 249) was already addressed in commit 592dc02.
207e397 to
a290db1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mongo/db/modules/eloq/SConscript (1)
201-211: Update store_deps to conditionally include S3 or GCS libraries.The
store_depsforROCKSDB_CLOUDhardcodes S3 libraries while GCS libraries remain commented out. This is inconsistent with the new GCS support added elsewhere in this file.Consider updating this section to conditionally include libraries based on
data_store_type_str:if data_store_type == DATA_STORE_TYPE_ROCKSDB_CLOUD: - store_deps = [ - # FindLibPath("rocksdb"), - FindLibPath("aws-cpp-sdk-core"), - FindLibPath("aws-cpp-sdk-kinesis"), - FindLibPath("aws-cpp-sdk-s3"), - # FindLibPath("google_cloud_cpp_common"), - # FindLibPath("google_cloud_cpp_storage"), - FindLibPath("rocksdb-cloud-aws"), - # FindLibPath("rocksdb-cloud-gcp"), - ] + if data_store_type_str == "ELOQDSS_ROCKSDB_CLOUD_S3": + store_deps = [ + FindLibPath("aws-cpp-sdk-core"), + FindLibPath("aws-cpp-sdk-kinesis"), + FindLibPath("aws-cpp-sdk-s3"), + FindLibPath("rocksdb-cloud-aws"), + ] + elif data_store_type_str == "ELOQDSS_ROCKSDB_CLOUD_GCS": + store_deps = [ + FindLibPath("google_cloud_cpp_common"), + FindLibPath("google_cloud_cpp_storage"), + FindLibPath("rocksdb-cloud-gcp"), + ] + else: + store_deps = []
🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/SConscript (2)
147-149: Consider removing redundant AWS includes.Lines 147-149 unconditionally add AWS SDK includes to all builds, but lines 165-171 already add these conditionally for S3 builds. This redundancy can be eliminated.
Since lines 165-171 correctly handle S3-specific includes, consider removing or commenting out lines 147-149:
FindIncludePath("mimalloc-2.1"), - FindIncludePath("aws/core"), - FindIncludePath("aws/kinesis"), - FindIncludePath("aws/s3"), + # FindIncludePath("aws/core"), + # FindIncludePath("aws/kinesis"), + # FindIncludePath("aws/s3"), # FindIncludePath("google/cloud/storage"),
248-253: Eliminate duplicate AWS SDK linker flags
Insrc/mongo/db/modules/eloq/SConscript(lines 204–209) the AWS SDK libraries are registered viastore_depsand again in the S3 conditional (lines 249–250). Wrap the FindLibPath calls in the conditional or remove the extraenv.Appendto avoid redundancy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/SConscript(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.
🔇 Additional comments (3)
src/mongo/db/modules/eloq/SConscript (3)
68-87: LGTM!The GCS data store type mapping and conditional CPPDEFINES are implemented correctly. The logic properly distinguishes between S3 and GCS variants while maintaining S3 as the default.
103-115: LGTM!The WITH_LOG_STATE handling is well-implemented with:
- Default value ("ROCKSDB") when the environment variable is unset
- All four expected log state types mapped correctly
- Explicit error handling with Exit(1) for invalid values
This properly addresses the previous review concern about missing default and validation.
159-174: LGTM!The conditional include path configuration correctly separates base RocksDB Cloud headers from backend-specific headers (S3 vs GCS). The use of list comprehension to filter out
Nonevalues is a good defensive pattern.
14d7a87 to
005cdb2
Compare
| #include "mongo/db/modules/eloq/log_service/include/log_server.h" | ||
| #else | ||
| #include "mongo/db/modules/eloq/eloq_log_service/include/log_server.h" | ||
| #include "mongo/db/modules/eloq/eloq_log_service/include/log_utils.h" |
There was a problem hiding this comment.
When we are using open log_service, it should
#include "mongo/db/modules/eloq/log_service/include/log_utils.h"
That's why we
#include "log_utils.h"
There was a problem hiding this comment.
We don't need log_utils.h when it is open_log
| if with_log_state == "MEMORY": | ||
| env.Append(CPPDEFINES=["LOG_STATE_TYPE_MEM"]) | ||
| elif with_log_state == "ROCKSDB": | ||
| env.Append(CPPDEFINES=["LOG_STATE_TYPE_RKDB"]) |
There was a problem hiding this comment.
Please don't use abbreviations.
There was a problem hiding this comment.
This abbreviations has been used in eloq_log_service... eloqkv and eloqsql.
A lot of revert works are needed to do if address your concern.
docs/how-to-compile.md
Outdated
| -B src/mongo/db/modules/eloq/build \ | ||
| -DCMAKE_INSTALL_PREFIX=$INSTALL_PREFIX \ | ||
| -DWITH_ROCKSDB_CLOUD=S3 \ | ||
| -DWITH_LOG_STATE=ROCKSDB_CLOUD_S3 \ |
There was a problem hiding this comment.
Since the open log_service doesn't support RocksDB-Cloud, what's the behavior of setting -DWITH_LOG_STATE=ROCKSDB_CLOUD_S3?
| # FindIncludePath("gperftools/heap-profiler.h") | ||
| ] | ||
| ) | ||
| _rkdb_cloud_includes = [ |
There was a problem hiding this comment.
Please don't use abbreviations.
|
|
||
| #if (defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3) || \ | ||
| defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_GCS)) | ||
| #define ELOQDS_RKDB_CLOUD 1 |
There was a problem hiding this comment.
Please don't use abbreviations.
| CMAKE_EXTRA_ARGS="${CMAKE_EXTRA_ARGS} -DWITH_LOG_STATE=ROCKSDB_CLOUD_GCS" | ||
| else | ||
| CMAKE_EXTRA_ARGS="${CMAKE_EXTRA_ARGS} -DWITH_ROCKSDB_CLOUD=OFF" | ||
| export WITH_ROCKSDB_CLOUD=0 |
There was a problem hiding this comment.
Why remove those exports?
There was a problem hiding this comment.
WITH_ROCKSDB_CLOUD is not used in open log
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/mongo/db/modules/eloq/SConscript (1)
201-215: Partition cloud deps by backend, filter out empty entries, and honor WITH_LOG_STATE
- In src/mongo/db/modules/eloq/SConscript, replace the single ROCKSDB_CLOUD
store_depslist with separate S3 vs GCS blocks and wrap with a helper that removesNoneentries.- Apply the same
_filter_none(base_deps + store_deps)before assigning toSYSLIBDEPS.- Extend the existing
env.Append(LIBS…)andCPPPATHlogic so that whenwith_log_stateis"ROCKSDB_CLOUD_S3"or"ROCKSDB_CLOUD_GCS", the corresponding AWS or GCS SDKs (and include paths) are linked even ifdata_store_typeis not explicitlyDATA_STORE_TYPE_ROCKSDB_CLOUD.src/mongo/db/modules/eloq/cmake/find_dependencies.cmake (3)
267-267: Fix typo in status message."requied" should be "required".
Apply this diff:
- message(STATUS "Dependencies: RocksDB is requied") + message(STATUS "Dependencies: RocksDB is required")
273-273: Fix typo in status message."Dependenies" should be "Dependencies".
Apply this diff:
- message(STATUS "Dependenies: RocksDB lib path ${ROCKSDB_LIB}") + message(STATUS "Dependencies: RocksDB lib path ${ROCKSDB_LIB}")
266-277: Remove duplicate RocksDB library discovery logic.This code block (lines 266-277) finds and adds the standard RocksDB library to
ROCKSDB_GLOBAL_LIBRARIES. However, lines 309-314 perform the exact same operation whenROCKSDB_CLOUD_SDK_FOUNDis OFF, searching for the same "rocksdb" library and adding it to the same list. WhileREMOVE_DUPLICATES(line 333) handles the duplicate entries, maintaining two discovery paths for the same library reduces maintainability and clarity.Remove this else block and rely on the existing logic at lines 307-316, which is better documented and fits naturally after the base include path discovery:
- else() - message(STATUS "Dependencies: RocksDB is requied") - find_library(ROCKSDB_LIB NAMES rocksdb) - - if(NOT ROCKSDB_LIB) - message(FATAL_ERROR "Dependencies: Failed to find all required lib path for RocksDB.") - endif() - message(STATUS "Dependenies: RocksDB lib path ${ROCKSDB_LIB}") - list(APPEND ROCKSDB_GLOBAL_LIBRARIES ${ROCKSDB_LIB} ) - - set(ROCKSDB_CLOUD_SDK_FOUND OFF) endif() # End of NEED_ROCKSDB_CLOUD_S3 / NEED_ROCKSDB_CLOUD_GCSNote: Setting
ROCKSDB_CLOUD_SDK_FOUNDto OFF is also redundant since it's already initialized to OFF at line 159 and only set to ON for cloud variants.
♻️ Duplicate comments (2)
concourse/scripts/build_tarball_open.bash (1)
218-222: Stop deleting${DEST_DIR}during cleanup.Removing
${DEST_DIR}still wipes the packaged artifacts, breaking the build output—this is the same critical issue previously flagged. Keep the cleanup limited to intermediate build directories so the installed payload survives.cd "$ELOQDOC_SRC" rm -rf src/mongo/db/modules/eloq/build rm -rf build -rm -rf ${DEST_DIR}src/mongo/db/modules/eloq/src/eloq_kv_engine.h (1)
30-35: Include log_utils.h for OPEN_LOG_SERVICE as wellWhen building with OPEN_LOG_SERVICE, log_utils.h should come from the open log_service path too. Currently it’s only included in the non-open path, which can break OPEN builds that rely on those utilities.
Apply this diff:
#ifdef OPEN_LOG_SERVICE -#include "mongo/db/modules/eloq/log_service/include/log_server.h" +#include "mongo/db/modules/eloq/log_service/include/log_server.h" +#include "mongo/db/modules/eloq/log_service/include/log_utils.h" #else #include "mongo/db/modules/eloq/eloq_log_service/include/log_server.h" #include "mongo/db/modules/eloq/eloq_log_service/include/log_utils.h" #endifBased on learnings
🧹 Nitpick comments (3)
src/mongo/db/modules/eloq/cmake/build_log_service.cmake (1)
127-141: Order Protobuf discovery before use; prefer PROTOBUF_LIBRARIES consistentlyMinor: ${PROTOBUF_LIBRARY} is added to LOG_LIB before calling find_package(Protobuf). Move find_package(Protobuf REQUIRED) above where it’s first used, and prefer ${PROTOBUF_LIBRARIES} for consistency with target_link_libraries.
Example minimal tweak:
- set(LOG_LIB + find_package(Protobuf REQUIRED) + set(LOG_LIB ${LOG_LIB} ${CMAKE_THREAD_LIBS_INIT} ${GFLAGS_LIBRARY} - ${PROTOBUF_LIBRARY} + ${PROTOBUF_LIBRARIES} ${GPERFTOOLS_LIBRARIES} ${LEVELDB_LIB} ${BRAFT_LIB} ${BRPC_LIB} dl z ) - -find_package(Protobuf REQUIRED)Based on learnings
src/mongo/db/modules/eloq/cmake/find_dependencies.cmake (2)
268-274: Consider fixing inconsistent indentation.Lines 268-274 use tabs for indentation, while the rest of the file consistently uses spaces. Standardizing indentation improves readability and prevents editor-specific display issues.
165-165: Consider making the status message more informative.Including the actual
WITH_LOG_STATEvalue would aid debugging and align with line 171's approach of displaying the triggering configuration value.Apply this diff:
- message(STATUS "Dependencies: RocksDB needed due to WITH_LOG_STATE.") + message(STATUS "Dependencies: RocksDB needed due to WITH_LOG_STATE=${WITH_LOG_STATE}.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
concourse/scripts/build_tarball.bash(2 hunks)concourse/scripts/build_tarball_open.bash(5 hunks)concourse/scripts/common.sh(4 hunks)docs/how-to-compile.md(2 hunks)src/mongo/db/modules/eloq/CMakeLists.txt(0 hunks)src/mongo/db/modules/eloq/SConscript(5 hunks)src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake(1 hunks)src/mongo/db/modules/eloq/cmake/build_log_service.cmake(1 hunks)src/mongo/db/modules/eloq/cmake/find_dependencies.cmake(3 hunks)src/mongo/db/modules/eloq/log_service(1 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(8 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.h(1 hunks)src/mongo/db/modules/eloq/store_handler(1 hunks)src/mongo/db/modules/eloq/tx_service(1 hunks)
💤 Files with no reviewable changes (1)
- src/mongo/db/modules/eloq/CMakeLists.txt
✅ Files skipped from review due to trivial changes (1)
- src/mongo/db/modules/eloq/tx_service
🚧 Files skipped from review as they are similar to previous changes (3)
- src/mongo/db/modules/eloq/store_handler
- docs/how-to-compile.md
- src/mongo/db/modules/eloq/log_service
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:95-115
Timestamp: 2025-09-25T12:09:01.276Z
Learning: The log states (LOG_STATE_TYPE_RKDB, LOG_STATE_TYPE_RKDB_S3, LOG_STATE_TYPE_RKDB_GCS) in the EloqDoc codebase are mutually exclusive - only one can be active at a time. The LOG_STATE_TYPE_RKDB_CLOUD macro is a helper that should only be defined for cloud variants (S3/GCS) and not when regular RKDB is active.
📚 Learning: 2025-09-25T11:58:50.446Z
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.
Applied to files:
concourse/scripts/build_tarball_open.bashsrc/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmakesrc/mongo/db/modules/eloq/src/eloq_kv_engine.hconcourse/scripts/build_tarball.bashsrc/mongo/db/modules/eloq/src/eloq_kv_engine.cppsrc/mongo/db/modules/eloq/cmake/build_log_service.cmakesrc/mongo/db/modules/eloq/cmake/find_dependencies.cmakeconcourse/scripts/common.sh
📚 Learning: 2025-09-25T12:24:06.434Z
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Applied to files:
src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmakesrc/mongo/db/modules/eloq/cmake/build_log_service.cmakesrc/mongo/db/modules/eloq/cmake/find_dependencies.cmake
📚 Learning: 2025-09-25T12:09:01.276Z
Learnt from: githubzilla
PR: eloqdata/eloqdoc#211
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:95-115
Timestamp: 2025-09-25T12:09:01.276Z
Learning: The log states (LOG_STATE_TYPE_RKDB, LOG_STATE_TYPE_RKDB_S3, LOG_STATE_TYPE_RKDB_GCS) in the EloqDoc codebase are mutually exclusive - only one can be active at a time. The LOG_STATE_TYPE_RKDB_CLOUD macro is a helper that should only be defined for cloud variants (S3/GCS) and not when regular RKDB is active.
Applied to files:
src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmakesrc/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
🔇 Additional comments (2)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)
119-121: Correct include path for rocksdb_cloud_config.hNeither “mongo/db/modules/eloq/log_service/include” nor “mongo/db/modules/eloq/eloq_log_service/include” contains rocksdb_cloud_config.h. Verify where this header lives and update the include accordingly.
Likely an incorrect or invalid review comment.
src/mongo/db/modules/eloq/cmake/find_dependencies.cmake (1)
187-194: LGTM! Cloud variant detection updated correctly.The transition from
USE_ROCKSDB_LOG_STATEtoWITH_LOG_STATEwith properSTREQUALcomparisons correctly identifies cloud requirements. This aligns well with the unifiedWITH_LOG_STATEarchitecture introduced in this PR.
| # Determine if any RocksDB related feature is enabled, thus requiring RocksDB | ||
| set(NEED_ROCKSDB OFF) | ||
| if(USE_ROCKSDB_LOG_STATE) | ||
| if(DEFINED WITH_LOG_STATE AND (WITH_LOG_STATE MATCHES "ROCKSDB|ROCKSDB_CLOUD_S3|ROCKSDB_CLOUD_GCS")) |
There was a problem hiding this comment.
Use exact string comparison for consistency and safety.
The MATCHES operator performs regex substring matching, which could incorrectly trigger RocksDB discovery if WITH_LOG_STATE contains values like "MY_ROCKSDB" or "USE_ROCKSDB_TEST". For consistency with the rest of the file (lines 188, 191, 198, 200 all use STREQUAL), use exact string comparison.
Apply this diff to use STREQUAL:
-if(DEFINED WITH_LOG_STATE AND (WITH_LOG_STATE MATCHES "ROCKSDB|ROCKSDB_CLOUD_S3|ROCKSDB_CLOUD_GCS"))
+if(DEFINED WITH_LOG_STATE AND (WITH_LOG_STATE STREQUAL "ROCKSDB" OR WITH_LOG_STATE STREQUAL "ROCKSDB_CLOUD_S3" OR WITH_LOG_STATE STREQUAL "ROCKSDB_CLOUD_GCS"))🤖 Prompt for AI Agents
In src/mongo/db/modules/eloq/cmake/find_dependencies.cmake around line 163, the
conditional uses MATCHES which does regex/substring matching and can misfire;
replace it with explicit STREQUAL comparisons for each allowed value (e.g. check
DEFINED WITH_LOG_STATE AND (WITH_LOG_STATE STREQUAL "ROCKSDB" OR WITH_LOG_STATE
STREQUAL "ROCKSDB_CLOUD_S3" OR WITH_LOG_STATE STREQUAL "ROCKSDB_CLOUD_GCS")) so
the check is exact and consistent with other lines in the file.
cb0180c to
7b8f2e5
Compare
Summary by CodeRabbit