Replace USE_ROCKSDB_LOG_STATE and WITH_ROCKSDB_CLOUD#127
Replace USE_ROCKSDB_LOG_STATE and WITH_ROCKSDB_CLOUD#127githubzilla merged 9 commits intoeloqdata:eloq-10.6.10from
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughUnifies log-state selection under a new WITH_LOG_STATE option (adds GCS), replaces legacy USE_ROCKSDB_LOG_STATE/WITH_ROCKSDB_CLOUD branches, updates build scripts and binary placement, refactors CMake RocksDB/cloud wiring and compile defs (LOG_STATE_TYPE_*), adjusts HA init for new guards and empty txlog handling, and bumps several submodule commits. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Node as HA Node
participant Cfg as Config
participant AWS as AWS SDK
participant GCP as GCP SDK
participant RKDB as RocksDB
participant TxSrv as TxLog Service
Note over Node,Cfg: Startup reads WITH_LOG_STATE
Node->>Cfg: read WITH_LOG_STATE
alt ROCKSDB_CLOUD_S3
Node->>AWS: init()
Node->>RKDB: init(cloud=S3)
else ROCKSDB_CLOUD_GCS
Node->>GCP: init()
Node->>RKDB: init(cloud=GCS)
else ROCKSDB
Node->>RKDB: init(local)
else MEMORY
Note over Node: use in-memory log state
end
Node->>Cfg: get txlog service list
alt service list non-empty
Node->>TxSrv: connect()
else empty
Note over Node,RKDB: initialize stand-alone bounded txlog
Node->>RKDB: configure(txlog path, ports)
end
Note over Node: Shutdown -> deinit AWS/GCP only if initialized
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 (12)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
storage/eloq/ha_eloq.cc (2)
2831-2850: Null‑safety and header issue: std::strlen on potentially null pointer.
- eloq_txlog_service_list can be null; std::strlen dereferences it.
- std::strlen requires , which isn’t included.
Use a null‑safe check and avoid the dependency.
Apply this diff:
- // If eloq_txlog_service_list is empty, it means bounded txlog service - if (std::strlen(eloq_txlog_service_list) == 0) + // If eloq_txlog_service_list is empty, it means bundled txlog service + if (!eloq_txlog_service_list || *eloq_txlog_service_list == '\0') @@ - // default rocksdb path is <data_home>/tx_log/rocksdb - if (strlen(eloq_txlog_rocksdb_storage_path) == 0) + // default rocksdb path is <data_home>/tx_log/rocksdb + if (!eloq_txlog_rocksdb_storage_path || + *eloq_txlog_rocksdb_storage_path == '\0')
2855-2860: Incorrect map indexing of ng_configs; potential out‑of‑range/default construction.ng_configs is an unordered_map keyed by ng_id; iterating with numeric 0..size()-1 and using operator[] risks creating empty entries and indexing [0] on an empty vector, leading to crashes.
Apply this diff to iterate deterministically and safely:
- for (uint32_t ng= 0; ng < ng_configs.size(); ng++) - { - // Use cc node port + 2 for log server - txlog_ports.emplace_back(ng_configs[ng][0].port_ + 2); - txlog_ips.emplace_back(ng_configs[ng][0].host_name_); - } + // Use cc node port + 2 for log server; order by ng_id for determinism + std::vector<uint32_t> ng_ids; + ng_ids.reserve(ng_configs.size()); + for (const auto &kv : ng_configs) ng_ids.push_back(kv.first); + std::sort(ng_ids.begin(), ng_ids.end()); + for (uint32_t ng_id : ng_ids) { + const auto &nodes = ng_configs.at(ng_id); + if (nodes.empty()) continue; + txlog_ports.emplace_back(nodes[0].port_ + 2); + txlog_ips.emplace_back(nodes[0].host_name_); + }storage/eloq/build_eloq_log_service.cmake (1)
182-206: Add the GCS include directory to the search path.Inside the GCS branch we discover
google/cloud/storage/client.h, but we never appendGCP_CS_INCLUDE_PATHtoROCKSDB_INCLUDE_PATH. As a result, the laterinclude_directories(${ROCKSDB_INCLUDE_PATH})call does not expose the Google Cloud Storage headers, so builds will fail whenever the GCS SDK lives outside the compiler’s default include paths (which is common in CI images and custom installs). Please mirror the S3 handling by pushing the GCS include directory intoROCKSDB_INCLUDE_PATHbefore we callinclude_directories.elseif(WITH_LOG_STATE STREQUAL "ROCKSDB_CLOUD_GCS") find_path(GCP_CS_INCLUDE_PATH google/cloud/storage/client.h) if((NOT GCP_CS_INCLUDE_PATH)) message(FATAL_ERROR "Fail to find google/cloud/storage include path") endif() message(STATUS "google/cloud/storage include path: ${GCP_CS_INCLUDE_PATH}") find_library(GCP_COMMON_LIB google_cloud_cpp_common) @@ message(STATUS "google_cloud_cpp_storage library: ${GCP_CS_LIB}") + set(ROCKSDB_INCLUDE_PATH ${ROCKSDB_INCLUDE_PATH} ${GCP_CS_INCLUDE_PATH}) + set(ROCKSDB_LIB ${ROCKSDB_LIB} ${GCP_COMMON_LIB}) set(ROCKSDB_LIB ${ROCKSDB_LIB} ${GCP_CS_LIB}) find_library(ROCKSDB_CLOUD_LIB NAMES rocksdb-cloud-gcp)
🧹 Nitpick comments (1)
storage/eloq/ha_eloq.cc (1)
211-214: Consolidate AWS SDK gating to avoid drift.The repeated preprocessor condition for AWS SDK init/deinit is correct but duplicated in several places. Consider centralizing it behind a single NEEDS_AWS_SDK define to reduce maintenance risk.
Also applies to: 2265-2284, 2297-2301, 2403-2411, 3311-3315
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
concourse/scripts/build_tarball.bash(3 hunks)concourse/scripts/build_tarball_open.bash(1 hunks)concourse/scripts/main.bash(1 hunks)concourse/scripts/main.ent.bash(2 hunks)concourse/scripts/pr.ent.bash(2 hunks)storage/eloq/build_eloq_log_service.cmake(3 hunks)storage/eloq/build_log_service.cmake(1 hunks)storage/eloq/ha_eloq.cc(10 hunks)storage/eloq/log_service(1 hunks)storage/eloq/tx_service(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
concourse/scripts/build_tarball.bash
[warning] 270-270: kv_type is referenced but not assigned.
(SC2154)
🔇 Additional comments (7)
storage/eloq/tx_service (1)
1-1: Confirm the new submodule commit is available upstreamDouble-check that
acf638f02dd9dca26f16a28c2e4b0b0ef38047cais pushed to the remotestorage/eloq/tx_servicerepo and contains the expected WITH_LOG_STATE updates; otherwise downstream clones/CI will fail on checkout.storage/eloq/log_service (1)
1-1: Confirm log_service submodule commit compatibilityPlease double-check that commit
4a208682f4f9dfa3986985a96a5d885a8dc3b738carries the WITH_LOG_STATE wiring and any build configuration updates required by this PR. We can’t inspect the submodule contents here, so a sanity check on that commit (and any needed coordination in dependent repos) would help avoid integration surprises.storage/eloq/ha_eloq.cc (1)
2877-2901: Macro matrix looks consistent; verify coverage for all intended modes.The LOG_STATE_TYPE_RKDB_*/MEM split and the nested LOG_STATE_TYPE_RKDB_CLOUD/S3 branches look correct functionally. After fixing the include guard above, this should compile for S3 and GCS. Please verify that build flags define exactly one of LOG_STATE_TYPE_RKDB_ALL or LOG_STATE_TYPE_MEM, and for cloud mode exactly one of LOG_STATE_TYPE_RKDB_CLOUD or non‑cloud.
Also applies to: 2926-2976
concourse/scripts/pr.ent.bash (2)
173-174: Unified log-state flag matches the new CMake contract.Switching to
-DWITH_LOG_STATE=ROCKSDB_CLOUD_S3keeps this build job aligned with the updated CMake interface.
196-196: Updated log_service build flag looks good.Using the consolidated
WITH_LOG_STATEargument here keeps the log-service build consistent with the rest of the pipeline.concourse/scripts/main.ent.bash (2)
152-153: Installer build now targets the unified log-state flag.The new
WITH_LOG_STATEsetting matches the CMake changes and removes the older ROCKSDB-specific toggles cleanly.
175-175: Log-service build is wired to the consolidated flag.This keeps the enterprise pipeline in lockstep with the new log-state selection logic.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
concourse/scripts/build_tarball.bash(3 hunks)storage/eloq/store_handler(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- storage/eloq/store_handler
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: githubzilla
PR: eloqdata/eloqsql#127
File: concourse/scripts/build_tarball_open.bash:136-136
Timestamp: 2025-09-25T11:33:33.221Z
Learning: The open log service (OPEN_LOG_SERVICE=ON) in concourse/scripts/build_tarball_open.bash only supports ROCKSDB as its log state and does not use the WITH_LOG_STATE parameter that was introduced to replace USE_ROCKSDB_LOG_STATE in other parts of the codebase.
Learnt from: githubzilla
PR: eloqdata/eloqsql#127
File: concourse/scripts/build_tarball.bash:226-226
Timestamp: 2025-09-25T11:36:38.412Z
Learning: In concourse/scripts/build_tarball.bash, the log_server build section (around lines 220-235) intentionally builds two variants of launch_sv: one with ROCKSDB_CLOUD_S3 (saved as launch_sv_s3) and another with local ROCKSDB (saved as launch_sv). The hardcoded -DWITH_LOG_STATE=ROCKSDB_CLOUD_S3 flag in the first build is intentional and should not be removed.
Learnt from: githubzilla
PR: eloqdata/eloqsql#127
File: storage/eloq/build_log_service.cmake:97-124
Timestamp: 2025-09-25T11:41:40.111Z
Learning: The `storage/eloq/build_log_service.cmake` file is specifically for the open log service build and only supports ROCKSDB as its log state backend. Unlike the enterprise version in `build_eloq_log_service.cmake`, it does not need to handle cloud-specific variants like ROCKSDB_CLOUD_S3 or ROCKSDB_CLOUD_GCS.
📚 Learning: 2025-09-25T11:36:38.412Z
Learnt from: githubzilla
PR: eloqdata/eloqsql#127
File: concourse/scripts/build_tarball.bash:226-226
Timestamp: 2025-09-25T11:36:38.412Z
Learning: In concourse/scripts/build_tarball.bash, the log_server build section (around lines 220-235) intentionally builds two variants of launch_sv: one with ROCKSDB_CLOUD_S3 (saved as launch_sv_s3) and another with local ROCKSDB (saved as launch_sv). The hardcoded -DWITH_LOG_STATE=ROCKSDB_CLOUD_S3 flag in the first build is intentional and should not be removed.
Applied to files:
concourse/scripts/build_tarball.bash
📚 Learning: 2025-09-25T11:33:33.221Z
Learnt from: githubzilla
PR: eloqdata/eloqsql#127
File: concourse/scripts/build_tarball_open.bash:136-136
Timestamp: 2025-09-25T11:33:33.221Z
Learning: The open log service (OPEN_LOG_SERVICE=ON) in concourse/scripts/build_tarball_open.bash only supports ROCKSDB as its log state and does not use the WITH_LOG_STATE parameter that was introduced to replace USE_ROCKSDB_LOG_STATE in other parts of the codebase.
Applied to files:
concourse/scripts/build_tarball.bash
📚 Learning: 2025-09-25T11:41:40.111Z
Learnt from: githubzilla
PR: eloqdata/eloqsql#127
File: storage/eloq/build_log_service.cmake:97-124
Timestamp: 2025-09-25T11:41:40.111Z
Learning: The `storage/eloq/build_log_service.cmake` file is specifically for the open log service build and only supports ROCKSDB as its log state backend. Unlike the enterprise version in `build_eloq_log_service.cmake`, it does not need to handle cloud-specific variants like ROCKSDB_CLOUD_S3 or ROCKSDB_CLOUD_GCS.
Applied to files:
concourse/scripts/build_tarball.bash
📚 Learning: 2025-09-25T11:40:48.717Z
Learnt from: githubzilla
PR: eloqdata/eloqsql#127
File: concourse/scripts/main.bash:164-165
Timestamp: 2025-09-25T11:40:48.717Z
Learning: The main.bash script in concourse/scripts/main.bash is for open log build, which only supports ROCKSDB as the log state option and does not need cloud-specific variants like ROCKSDB_CLOUD_S3.
Applied to files:
concourse/scripts/build_tarball.bash
🧬 Code graph analysis (1)
concourse/scripts/build_tarball.bash (1)
concourse/scripts/build_tarball_open.bash (1)
copy_libraries(107-115)
githubzilla
force-pushed
the
remove_use_rocksdb_cloud
branch
from
fb55908 to
1850b17
Compare
2 participants
Summary by CodeRabbit
New Features
Refactor
Chores