Conversation
WalkthroughAdds eloqstore sources and build tasks to multiple Concourse pipelines, updates build tasks to accept the new input, and migrates build script paths and submodule logic from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
yi-xmu
force-pushed
the
support_build_with_eloqstore
branch
from
b010d3d to
4a49225
Compare
![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)
concourse/pipeline/build_nightly_tarball.yml (1)
100-113: Task naming is inconsistent across pipelines for ELOQDSS_ELOQSTORE data store type.The nightly and release pipelines both use
build-eloqdss-eloqstore-typefor ELOQDSS_ELOQSTORE tasks, but the debug pipeline usesbuild-eloqstore-type(without theeloqdss-prefix). This creates an inconsistency across pipelines for the same data store type.Should the debug tarball task be renamed to
build-eloqdss-eloqstore-typeto match the nightly and release pipelines?concourse/scripts/build_tarball.bash (1)
79-82:build_upload_log_srvfunction missing cases for ROCKSDB and ELOQSTORE DATA_STORE_TYPE values.The
build_upload_log_srvfunction (lines 279–283) only handlesELOQDSS_ROCKSDB_CLOUD_S3andELOQDSS_ROCKSDB_CLOUD_GCScases by setting-DWITH_LOG_STATE. WhenBUILD_LOG_SRV=trueis combined withDATA_STORE_TYPE=ELOQDSS_ROCKSDBorELOQDSS_ELOQSTORE, the function doesn't setWITH_LOG_STATEin cmake_args, causing cmake to fail with "Unknown WITH_LOG_STATE" error (storage/eloq/CMakeLists.txt line 42). These combinations are actively used in pipelines (build_release_tarball.yml).Add cases for ROCKSDB and ELOQSTORE:
elif [ "$ds_type" = "ELOQDSS_ROCKSDB" ]; then cmake_args="$cmake_args -DWITH_LOG_STATE=ROCKSDB" elif [ "$ds_type" = "ELOQDSS_ELOQSTORE" ]; then cmake_args="$cmake_args -DWITH_LOG_STATE=ROCKSDB_CLOUD_GCS"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
concourse/pipeline/build_debug_tarball.yml(4 hunks)concourse/pipeline/build_nightly_tarball.yml(17 hunks)concourse/pipeline/build_release_tarball.yml(23 hunks)concourse/scripts/build_tarball.bash(6 hunks)concourse/tasks/build_nightly_tarball.yml(1 hunks)concourse/tasks/build_release_tarball.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 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:41:40.111Z
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 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.bashconcourse/pipeline/build_nightly_tarball.ymlconcourse/pipeline/build_debug_tarball.ymlconcourse/pipeline/build_release_tarball.yml
📚 Learning: 2025-09-25T11:40:48.717Z
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 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.bashconcourse/pipeline/build_nightly_tarball.ymlconcourse/pipeline/build_debug_tarball.ymlconcourse/pipeline/build_release_tarball.yml
📚 Learning: 2025-11-07T05:12:43.208Z
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2770-2781
Timestamp: 2025-11-07T05:12:43.208Z
Learning: In eloqdata/eloqsql, storage/eloq/ha_eloq.cc (KV_ELOQDS path): for DATA_STORE_TYPE_ELOQDSS_ROCKSDB (local RocksDB), each eloqsql instance owns its own RocksDB instance, and passing create_if_missing=true to DataStoreService::StartService is intentional and safe.
Applied to files:
concourse/scripts/build_tarball.bash
📚 Learning: 2025-09-25T11:33:33.221Z
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 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.bashconcourse/pipeline/build_nightly_tarball.ymlconcourse/pipeline/build_debug_tarball.ymlconcourse/pipeline/build_release_tarball.yml
🧬 Code graph analysis (1)
concourse/scripts/build_tarball.bash (1)
concourse/scripts/build_tarball_open.bash (1)
copy_libraries(107-115)
🔇 Additional comments (10)
concourse/tasks/build_release_tarball.yml (1)
8-8: Input declaration aligns with pipeline changes.Adding
eloqstore_srcas an input is consistent with the pipeline's newget: eloqstore_srcsteps and the build script's conditional eloqstore setup.concourse/scripts/build_tarball.bash (3)
25-45: Submodule orchestration and data_substrate paths look correct.The conditional eloqstore setup (lines 38–45) properly gates on
DATA_STORE_TYPEand follows the same pattern as logservice and raft_host_manager. The git submodule operations are appropriate for pulling transitive dependencies.
279-283: Log server build derives flags fromDATA_STORE_TYPE, notWITH_LOG_STATE.The
build_upload_log_srvfunction checksds_type(which isDATA_STORE_TYPE) to set CMake flags forROCKSDB_CLOUD_S3andROCKSDB_CLOUD_GCS, but does not reference theWITH_LOG_STATEparameter. IfWITH_LOG_STATEis meant to control log server configuration, this function should be updated to use it.Does
WITH_LOG_STATEneed to be threaded through tobuild_upload_log_srv, or is deriving it fromDATA_STORE_TYPEthe correct approach?
64-66: Verify intentional use of differentWITH_LOG_STATEbackends across build pipeline types.The
WITH_LOG_STATEvalues forELOQDSS_ELOQSTOREbuilds differ across pipelines: debug usesROCKSDB, nightly usesROCKSDB_CLOUD_S3, and release usesROCKSDB_CLOUD_GCS(at lines 66, 112/175/238/301, and 124/194/263/333/401/466 respectively). This pattern suggests the differences may be intentional—debug builds use local-only storage while nightly and release builds target cloud-backed deployments. However, without comments or documentation explaining this design choice, it remains unclear if these differences are deliberate or inconsistently configured. Please confirm whether this design is intentional and, if so, add comments to the pipeline files clarifying why different backends are used for different build types.concourse/tasks/build_nightly_tarball.yml (1)
8-8: Input declaration is consistent across tarball task definitions.concourse/pipeline/build_nightly_tarball.yml (1)
46-52: Resource and dependency structure is well-organized.The new
eloqstore_src_mainresource is properly defined and consistently fetched across all OS variant jobs. The pattern mirrors the existing logservice and raft_host_manager resources.Also applies to: 65-66, 128-129, 191-192, 254-255
concourse/pipeline/build_debug_tarball.yml (1)
28-34: Resource definition and dependency fetching are consistent with other pipelines.Also applies to: 47-48
concourse/pipeline/build_release_tarball.yml (3)
54-60: Resource and dependency structure follows established patterns.The
eloqstore_src_mainresource is properly integrated across all OS variant jobs, mirroring the structure used in nightly and debug pipelines.Also applies to: 73-74, 143-144, 212-213, 282-283, 352-353, 417-418
78-78: Task naming refactor improves consistency.Renaming
build-rocksdb-cloud-s3-typetobuild-eloqdss-rocksdb-cloud-s3-typeadds a consistenteloqdss-prefix, aligning it with other ELOQDSS variant tasks. This is a good refactoring.Also applies to: 148-148, 217-217, 287-287, 357-357, 422-422
112-127: Verify the intendedWITH_LOG_STATEbackend for ELOQDSS_ELOQSTORE release builds.The
build-eloqdss-eloqstore-typetasks across all six OS variants consistently useWITH_LOG_STATE: ROCKSDB_CLOUD_GCS, which differs from debug builds (ROCKSDB) and nightly builds (ROCKSDB_CLOUD_S3) for the sameDATA_STORE_TYPE. This inconsistency appears across lines 112-127, 182-197, 251-266, 321-336, 389-403, and 454-468.Confirm whether GCS is intentional for release tarball eloqstore builds, or whether it should match the nightly value (
ROCKSDB_CLOUD_S3).
| - task: build-eloqstore-type | ||
| file: eloqsql_src/concourse/tasks/build_debug_tarball.yml | ||
| image: image_ubuntu2204 | ||
| params: | ||
| ASAN: "ON" | ||
| TAGGED: "" | ||
| GIT_SSH_KEY: ((git-key)) | ||
| AWS_ACCESS_KEY_ID: ((aws_access_key_id)) | ||
| AWS_DEFAULT_REGION: ((aws-region)) | ||
| AWS_SECRET_ACCESS_KEY: ((aws_secret_access_key)) | ||
| BUILD_LOG_SRV: "true" | ||
| BUILD_TYPE: Debug | ||
| DATA_STORE_TYPE: ELOQDSS_ELOQSTORE | ||
| OUT_NAME: debug | ||
| WITH_LOG_STATE: ROCKSDB | ||
| CLOUDFRONT_DIST: ((cloudfront-dist)) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find pipeline files (nightly and release)
find . -type f -name "*pipeline*" -o -name "*nightly*" -o -name "*release*" | head -20Repository: eloqdata/eloqsql
Length of output: 176
🏁 Script executed:
#!/bin/bash
# Search for task names containing "eloqstore" or "eloqdss-eloqstore"
rg "build-eloq.*store.*type" --type yamlRepository: eloqdata/eloqsql
Length of output: 973
Task naming differs from nightly and release pipelines.
The task build-eloqstore-type is inconsistent with nightly (build_nightly_tarball.yml) and release (build_release_tarball.yml) pipelines, which both use build-eloqdss-eloqstore-type. Consider renaming to align with the established naming convention across build pipelines.
🤖 Prompt for AI Agents
In concourse/pipeline/build_debug_tarball.yml around lines 52 to 68, the task
name "build-eloqstore-type" is inconsistent with nightly/release pipelines;
rename the task to "build-eloqdss-eloqstore-type" to match the established
convention, update any task references in this pipeline or other
pipelines/parent jobs that call this task to the new name, and run a quick
pipeline config lint or dry-run to ensure no remaining references break.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.