Conversation
WalkthroughReplaces RocksDB cloud S3 settings with new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-09-25T11:41:40.111ZApplied to files:
📚 Learning: 2025-11-07T05:12:43.208ZApplied to files:
📚 Learning: 2025-09-25T11:40:48.717ZApplied to files:
🧬 Code graph analysis (1)sql/sys_vars.cc (2)
🔇 Additional comments (37)
✏️ Tip: You can disable this entire section by setting 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
🤖 Fix all issues with AI agents
In `@concourse/scripts/main.ent.bash`:
- Around line 103-104: The sed replacement for eloq_store_cloud_access_key
contains an extra space before the equals sign causing the INI key to be written
with a trailing space; update the sed expression that targets
eloq_store_cloud_access_key (the first sed command) to remove the extraneous
space so it produces "eloq_store_cloud_access_key=${ELOQ_AWS_ACCESS_KEY_ID}"
(matching the pattern used for eloq_store_cloud_secret_key) and ensure the
variable ELOQ_AWS_ACCESS_KEY_ID is inserted without added whitespace.
In `@concourse/scripts/mtr_bootstrap.cnf`:
- Line 8: Replace the hard-coded developer-specific absolute path assigned to
eloq_config in mtr_bootstrap.cnf with a repo-relative or environment-driven
value: change the assignment of the variable eloq_config so it builds the path
relative to the repository root (or reads a base directory from an environment
variable like ELOQ_BASE_DIR) and joins it with the config filename
(mtr_bootstrap_ds.cnf) instead of embedding /home/tianxj/.... Update any
bootstrap logic that reads eloq_config to fallback to a sensible default if the
env var is not set.
In `@concourse/scripts/mtr_multi_bootstrap.cnf`:
- Line 8: The eloq_config setting currently contains a developer-specific
absolute path (/home/tianxj/...) which is not portable; update the eloq_config
entry to use either a repository-relative path (e.g., relative to the script
location) or read from an environment variable (e.g., ELOQ_CONFIG) so the
bootstrap can run on other machines; modify the eloq_config assignment in
concourse/scripts/mtr_multi_bootstrap.cnf to reference the chosen repo-relative
location or an env var and document the expected variable if used.
In `@concourse/scripts/pr.ent.bash`:
- Around line 124-125: The first sed replacement adds an extra space after the =
in the eloq_store_cloud_access_key assignment which can introduce trailing
whitespace; update the sed expression for eloq_store_cloud_access_key to remove
that space so it matches the eloq_store_cloud_secret_key pattern (use the same
substitution style and the ELOQ_AWS_ACCESS_KEY_ID variable), ensuring both
replacements produce "key=VALUE" without extra whitespace in dss_server.ini.
- Around line 105-117: The config edits in pr.ent.bash update templates and sed
replacements under eloqsql_src (update_config_template for mtr_bootstrap_ds.cnf
and mtr_multi_bootstrap_ds.cnf and the sed lines that set eloq_config and
hm_bin_path), but later runtime/test steps still reference eloqsql_pr; to fix,
choose one root and make all references consistent—either change these
occurrences of "$WORKSPACE/eloqsql_src/..." to "$WORKSPACE/eloqsql_pr/..." (for
mtr_bootstrap_ds.cnf, mtr_multi_bootstrap_ds.cnf and hm_bin_path) or update the
downstream MTR/bootstrap and dss_server invocation to point at eloqsql_src;
ensure the same variable/root is used for update_config_template, the sed
replacements (eloq_config and hm_bin_path), and any later test/runtime
references so the configs actually used by tests are the ones you edited.
In `@storage/eloq/eloq_system_handler.cpp`:
- Around line 38-46: The current branch returns early when my_thread_init()
fails, leaving shutdown_ false and the internal thread potentially unjoined;
change the failure path in the my_thread_init() check (and the analogous block
at lines 105-115) to set shutdown_ = true and then ensure the worker thread is
joined if joinable (call Shutdown() or thread_.join()/equivalent) before
returning so no further work can be enqueued and the thread is properly cleaned
up; update the failure handling around my_thread_init(), shutdown_, and the
thread join logic so both init failures and normal shutdown follow the same join
semantics.
♻️ Duplicate comments (3)
concourse/scripts/mtr_multi_bootstrap_ds.cnf (1)
15-16: Same MinIO-credential concern applies here.
Avoid hardcoding access keys if env/CI substitution is supported.storage/eloq/mysql-test/mono_multi/data_substrate3.cnf (1)
15-18: Same credential + empty DSS config path concern applies here.
Please ensure the empty path is valid and credentials aren’t inadvertently reused outside CI.storage/eloq/mysql-test/mono_multi/data_substrate2.cnf (1)
15-18: Same credential + empty DSS config path concern applies here.
Please ensure the empty path is valid and credentials aren’t inadvertently reused outside CI.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
concourse/scripts/dss_server.iniconcourse/scripts/main.ent.bashconcourse/scripts/mtr_bootstrap.cnfconcourse/scripts/mtr_bootstrap_ds.cnfconcourse/scripts/mtr_multi_bootstrap.cnfconcourse/scripts/mtr_multi_bootstrap_ds.cnfconcourse/scripts/pr.ent.bashdata_substratestorage/eloq/eloq_system_handler.cppstorage/eloq/mysql-test/mono_basic/data_substrate.cnfstorage/eloq/mysql-test/mono_main/data_substrate.cnfstorage/eloq/mysql-test/mono_multi/data_substrate1.cnfstorage/eloq/mysql-test/mono_multi/data_substrate2.cnfstorage/eloq/mysql-test/mono_multi/data_substrate3.cnf
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2571-2623
Timestamp: 2025-11-07T07:10:40.633Z
Learning: Repo: eloqdata/eloqsql PR: 154
File/Area: storage/eloq/ha_eloq.cc (eloq_init_func cluster membership resolution)
Learning: In this repo’s ng_config, the local node is guaranteed to be a candidate (NodeConfig::is_candidate_ == true). Therefore, assigning native_ng_id only when is_candidate_ is true is acceptable; the key improvement is to break early from both loops once the local node is found.
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.
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.
📚 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/dss_server.inistorage/eloq/mysql-test/mono_multi/data_substrate3.cnfstorage/eloq/mysql-test/mono_basic/data_substrate.cnfconcourse/scripts/mtr_multi_bootstrap_ds.cnfstorage/eloq/mysql-test/mono_main/data_substrate.cnfconcourse/scripts/mtr_bootstrap_ds.cnfconcourse/scripts/pr.ent.bashstorage/eloq/mysql-test/mono_multi/data_substrate1.cnfconcourse/scripts/main.ent.bashstorage/eloq/mysql-test/mono_multi/data_substrate2.cnf
📚 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/dss_server.inistorage/eloq/mysql-test/mono_multi/data_substrate3.cnfstorage/eloq/mysql-test/mono_basic/data_substrate.cnfconcourse/scripts/mtr_multi_bootstrap_ds.cnfstorage/eloq/mysql-test/mono_main/data_substrate.cnfconcourse/scripts/mtr_bootstrap_ds.cnfconcourse/scripts/pr.ent.bashstorage/eloq/mysql-test/mono_multi/data_substrate1.cnfconcourse/scripts/main.ent.bashstorage/eloq/mysql-test/mono_multi/data_substrate2.cnf
📚 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:
storage/eloq/mysql-test/mono_basic/data_substrate.cnfconcourse/scripts/mtr_multi_bootstrap_ds.cnfstorage/eloq/mysql-test/mono_main/data_substrate.cnfconcourse/scripts/mtr_bootstrap_ds.cnfconcourse/scripts/pr.ent.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/mtr_bootstrap_ds.cnfconcourse/scripts/pr.ent.bashconcourse/scripts/main.ent.bash
📚 Learning: 2025-11-07T07:10:40.633Z
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2571-2623
Timestamp: 2025-11-07T07:10:40.633Z
Learning: Repo: eloqdata/eloqsql PR: 154
File/Area: storage/eloq/ha_eloq.cc (eloq_init_func cluster membership resolution)
Learning: In this repo’s ng_config, the local node is guaranteed to be a candidate (NodeConfig::is_candidate_ == true). Therefore, assigning native_ng_id only when is_candidate_ is true is acceptable; the key improvement is to break early from both loops once the local node is found.
Applied to files:
concourse/scripts/mtr_multi_bootstrap.cnf
🧬 Code graph analysis (1)
concourse/scripts/pr.ent.bash (2)
concourse/scripts/main.ent.bash (1)
update_config_template(49-69)concourse/scripts/main.bash (1)
update_config_template(44-64)
🔇 Additional comments (15)
data_substrate (1)
1-1: Verify the submodule update against PR objectives.Confirm that the data_substrate submodule update (from commit 25dd6a39cedb036e8ce849509245769fbcea48c7 to c2b1c26101ba4bace22b14fe348a6d5d454f5d6d) contains expected changes supporting the stated PR goals and doesn't introduce breaking changes. This requires examining the commit details in the tx_service repository to confirm configuration updates, template modifications, and backward compatibility.
storage/eloq/mysql-test/mono_multi/data_substrate1.cnf (2)
9-9: LGTM for the object-store service URL update.
This aligns the txlog config with the new object-store style.
15-18: These are test fixtures with CI-driven substitution already in place.The hardcoded
minioadmincredentials (lines 15–16) are standard MinIO test defaults appropriate for local test config templates. The concourse CI infrastructure already substitutes credentials via environment variables (ELOQ_AWS_ACCESS_KEY_ID,ELOQ_AWS_SECRET_KEY) and populates the emptyeloq_dss_config_file_path(line 18) with a resolved path during execution. No changes needed.concourse/scripts/mtr_multi_bootstrap_ds.cnf (1)
9-9: LGTM for the object-store service URL update.storage/eloq/mysql-test/mono_multi/data_substrate3.cnf (1)
9-9: LGTM for the object-store service URL update.storage/eloq/eloq_system_handler.cpp (1)
55-65: Logging additions look good.
The DLOGs provide helpful lifecycle and work-tracing without affecting release builds.Also applies to: 68-70, 101-101
storage/eloq/mysql-test/mono_multi/data_substrate2.cnf (1)
9-9: LGTM for the object-store service URL update.storage/eloq/mysql-test/mono_main/data_substrate.cnf (1)
10-21: LGTM for ELOQSTORE config swap.
Keys and endpoints look consistent with the new eloq_store_* layout.storage/eloq/mysql-test/mono_basic/data_substrate.cnf (1)
10-21: LGTM for ELOQSTORE config swap.
Configuration aligns with the new eloq_store_* keys.concourse/scripts/dss_server.ini (1)
9-13: LGTM for ELOQSTORE settings.
Config looks consistent with the new eloq_store_* layout.concourse/scripts/mtr_bootstrap_ds.cnf (1)
8-9: ELOQSTORE config defaults look consistent.The new txlog object-store URL and
eloq_store_*values align with the updated MinIO-based setup.Also applies to: 15-24
concourse/scripts/main.ent.bash (2)
57-68: Template substitutions align with new eloq_store keys.The MinIO endpoint, object-store URL, and eloq_store substitutions look consistent with the ELOQSTORE migration.
131-132: ELOQSTORE build flags applied consistently.Both the main build and dss_server build now target
ELOQDSS_ELOQSTOREas intended.Also applies to: 150-150
concourse/scripts/pr.ent.bash (2)
78-89: Template substitutions updated for eloq_store.The new MinIO endpoint, object-store URL, and eloq_store substitutions align with the migration.
153-154: ELOQSTORE build flags applied for dss_server.The CMake switches to
ELOQDSS_ELOQSTOREare consistent across the main build and the dss_server build.Also applies to: 172-172
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
concourse/scripts/mtr_bootstrap.cnf
Outdated
| thread_pool_dedicated_listener=1 | ||
|
|
||
| eloq_config= No newline at end of file | ||
| eloq_config=/home/tianxj/hive/eloqdata/eloqsql/concourse/scripts/mtr_bootstrap_ds.cnf |
There was a problem hiding this comment.
Avoid developer-specific absolute paths.
/home/tianxj/... will break in CI and on other machines. Use a repo‑relative path or an env-configured base path instead.
💡 Possible fix (adjust to your bootstrap resolution rules)
-eloq_config=/home/tianxj/hive/eloqdata/eloqsql/concourse/scripts/mtr_bootstrap_ds.cnf
+eloq_config=concourse/scripts/mtr_bootstrap_ds.cnf📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| eloq_config=/home/tianxj/hive/eloqdata/eloqsql/concourse/scripts/mtr_bootstrap_ds.cnf | |
| eloq_config=concourse/scripts/mtr_bootstrap_ds.cnf |
🤖 Prompt for AI Agents
In `@concourse/scripts/mtr_bootstrap.cnf` at line 8, Replace the hard-coded
developer-specific absolute path assigned to eloq_config in mtr_bootstrap.cnf
with a repo-relative or environment-driven value: change the assignment of the
variable eloq_config so it builds the path relative to the repository root (or
reads a base directory from an environment variable like ELOQ_BASE_DIR) and
joins it with the config filename (mtr_bootstrap_ds.cnf) instead of embedding
/home/tianxj/.... Update any bootstrap logic that reads eloq_config to fallback
to a sensible default if the env var is not set.
| # config mtr_bootstrap.cnf | ||
| update_config_template "$WORKSPACE/eloqsql_pr/concourse/scripts/mtr_bootstrap_ds.cnf" | ||
| sed -i "s|eloq_config=.*|eloq_config=$WORKSPACE/eloqsql_pr/concourse/scripts/mtr_bootstrap_ds.cnf|g" $WORKSPACE/eloqsql_pr/concourse/scripts/mtr_bootstrap.cnf | ||
| sed -i "s|hm_bin_path=.*|hm_bin_path=$WORKSPACE/eloqsql_pr/bld/data_substrate/host_manager|g" $WORKSPACE/eloqsql_pr/concourse/scripts/mtr_bootstrap_ds.cnf | ||
| update_config_template "$WORKSPACE/eloqsql_src/concourse/scripts/mtr_bootstrap_ds.cnf" | ||
| sed -i "s|eloq_config=.*|eloq_config=$WORKSPACE/eloqsql_src/concourse/scripts/mtr_bootstrap_ds.cnf|g" $WORKSPACE/eloqsql_src/concourse/scripts/mtr_bootstrap.cnf | ||
| sed -i "s|hm_bin_path=.*|hm_bin_path=$WORKSPACE/eloqsql_src/bld/data_substrate/host_manager|g" $WORKSPACE/eloqsql_src/concourse/scripts/mtr_bootstrap_ds.cnf | ||
| echo "mtr_bootstrap_ds.cnf" | ||
| cat $WORKSPACE/eloqsql_pr/concourse/scripts/mtr_bootstrap_ds.cnf | ||
| cat $WORKSPACE/eloqsql_src/concourse/scripts/mtr_bootstrap_ds.cnf | ||
|
|
||
| #config mtr_multi_bootstrap.cnf | ||
| update_config_template "$WORKSPACE/eloqsql_pr/concourse/scripts/mtr_multi_bootstrap_ds.cnf" | ||
| sed -i "s|eloq_config=.*|eloq_config=$WORKSPACE/eloqsql_pr/concourse/scripts/mtr_multi_bootstrap_ds.cnf|g" $WORKSPACE/eloqsql_pr/concourse/scripts/mtr_multi_bootstrap.cnf | ||
| sed -i "s|hm_bin_path=.*|hm_bin_path=$WORKSPACE/eloqsql_pr/bld/data_substrate/host_manager|g" $WORKSPACE/eloqsql_pr/concourse/scripts/mtr_multi_bootstrap_ds.cnf | ||
| update_config_template "$WORKSPACE/eloqsql_src/concourse/scripts/mtr_multi_bootstrap_ds.cnf" | ||
| sed -i "s|eloq_config=.*|eloq_config=$WORKSPACE/eloqsql_src/concourse/scripts/mtr_multi_bootstrap_ds.cnf|g" $WORKSPACE/eloqsql_src/concourse/scripts/mtr_multi_bootstrap.cnf | ||
| sed -i "s|hm_bin_path=.*|hm_bin_path=$WORKSPACE/eloqsql_src/bld/data_substrate/host_manager|g" $WORKSPACE/eloqsql_src/concourse/scripts/mtr_multi_bootstrap_ds.cnf | ||
| echo "mtr_multi_bootstrap_ds.cnf" | ||
| cat $WORKSPACE/eloqsql_pr/concourse/scripts/mtr_multi_bootstrap_ds.cnf | ||
| cat $WORKSPACE/eloqsql_src/concourse/scripts/mtr_multi_bootstrap_ds.cnf |
There was a problem hiding this comment.
Config edits now target eloqsql_src while tests still use eloqsql_pr.
These edits update configs under eloqsql_src, but later commands (e.g., the MTR bootstrap and dss_server config usage) still reference eloqsql_pr, so the configs actually used by tests may remain unmodified. Please align all config edits and runtime references to the same root.
If the PR flow should continue using eloqsql_pr, consider switching these paths back:
🔧 Possible fix (align to eloqsql_pr)
-update_config_template "$WORKSPACE/eloqsql_src/concourse/scripts/mtr_bootstrap_ds.cnf"
-sed -i "s|eloq_config=.*|eloq_config=$WORKSPACE/eloqsql_src/concourse/scripts/mtr_bootstrap_ds.cnf|g" $WORKSPACE/eloqsql_src/concourse/scripts/mtr_bootstrap.cnf
-sed -i "s|hm_bin_path=.*|hm_bin_path=$WORKSPACE/eloqsql_src/bld/data_substrate/host_manager|g" $WORKSPACE/eloqsql_src/concourse/scripts/mtr_bootstrap_ds.cnf
+update_config_template "$WORKSPACE/eloqsql_pr/concourse/scripts/mtr_bootstrap_ds.cnf"
+sed -i "s|eloq_config=.*|eloq_config=$WORKSPACE/eloqsql_pr/concourse/scripts/mtr_bootstrap_ds.cnf|g" $WORKSPACE/eloqsql_pr/concourse/scripts/mtr_bootstrap.cnf
+sed -i "s|hm_bin_path=.*|hm_bin_path=$WORKSPACE/eloqsql_pr/bld/data_substrate/host_manager|g" $WORKSPACE/eloqsql_pr/concourse/scripts/mtr_bootstrap_ds.cnf🤖 Prompt for AI Agents
In `@concourse/scripts/pr.ent.bash` around lines 105 - 117, The config edits in
pr.ent.bash update templates and sed replacements under eloqsql_src
(update_config_template for mtr_bootstrap_ds.cnf and mtr_multi_bootstrap_ds.cnf
and the sed lines that set eloq_config and hm_bin_path), but later runtime/test
steps still reference eloqsql_pr; to fix, choose one root and make all
references consistent—either change these occurrences of
"$WORKSPACE/eloqsql_src/..." to "$WORKSPACE/eloqsql_pr/..." (for
mtr_bootstrap_ds.cnf, mtr_multi_bootstrap_ds.cnf and hm_bin_path) or update the
downstream MTR/bootstrap and dss_server invocation to point at eloqsql_src;
ensure the same variable/root is used for update_config_template, the sed
replacements (eloq_config and hm_bin_path), and any later test/runtime
references so the configs actually used by tests are the ones you edited.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@concourse/scripts/pr.ent.bash`:
- Around line 169-174: The second cmake invocation overwrites the first, so
ensure only the intended ELOQDSS_ELOQSTORE configuration is used for building
dss_server: remove the second cmake line that passes
-DWITH_DATA_STORE=ELOQDSS_ROCKSDB_CLOUD_S3 or alternatively create a separate
build directory (e.g., bld_rocks or bld_elq) and run the respective cmake
command there; locate the cmake calls in the dss_server build block (mkdir bld
&& cd bld and the two cmake -DWITH_DATA_STORE=... lines) and keep only the cmake
call that sets -DWITH_DATA_STORE=ELOQDSS_ELOQSTORE or split into distinct build
directories before cmake to avoid configuration override.
♻️ Duplicate comments (2)
concourse/scripts/pr.ent.bash (2)
105-117: Align edited bootstrap configs with the paths used at runtime.
These edits targeteloqsql_src, but later MTR/bootstrap usage still points ateloqsql_pr, so the tests may consume unmodified configs.🔧 Possible fix (align to eloqsql_pr)
-update_config_template "$WORKSPACE/eloqsql_src/concourse/scripts/mtr_bootstrap_ds.cnf" -sed -i "s|eloq_config=.*|eloq_config=$WORKSPACE/eloqsql_src/concourse/scripts/mtr_bootstrap_ds.cnf|g" $WORKSPACE/eloqsql_src/concourse/scripts/mtr_bootstrap.cnf -sed -i "s|hm_bin_path=.*|hm_bin_path=$WORKSPACE/eloqsql_src/bld/data_substrate/host_manager|g" $WORKSPACE/eloqsql_src/concourse/scripts/mtr_bootstrap_ds.cnf +update_config_template "$WORKSPACE/eloqsql_pr/concourse/scripts/mtr_bootstrap_ds.cnf" +sed -i "s|eloq_config=.*|eloq_config=$WORKSPACE/eloqsql_pr/concourse/scripts/mtr_bootstrap_ds.cnf|g" $WORKSPACE/eloqsql_pr/concourse/scripts/mtr_bootstrap.cnf +sed -i "s|hm_bin_path=.*|hm_bin_path=$WORKSPACE/eloqsql_pr/bld/data_substrate/host_manager|g" $WORKSPACE/eloqsql_pr/concourse/scripts/mtr_bootstrap_ds.cnf
121-127: Remove whitespace ineloq_store_cloud_access_keyassignment.
The extra space after=can trip parsers that don't trim whitespace and is inconsistent with the secret key line.🔧 Proposed fix
-sed -i "s/eloq_store_cloud_access_key.*=.\+/eloq_store_cloud_access_key =${ELOQ_AWS_ACCESS_KEY_ID}/g" $WORKSPACE/eloqsql_src/concourse/scripts/dss_server.ini +sed -i "s/eloq_store_cloud_access_key.*=.\+/eloq_store_cloud_access_key=${ELOQ_AWS_ACCESS_KEY_ID}/g" $WORKSPACE/eloqsql_src/concourse/scripts/dss_server.ini
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
concourse/scripts/mtr_bootstrap.cnfconcourse/scripts/mtr_multi_bootstrap.cnfconcourse/scripts/pr.ent.bash
✅ Files skipped from review due to trivial changes (1)
- concourse/scripts/mtr_multi_bootstrap.cnf
🚧 Files skipped from review as they are similar to previous changes (1)
- concourse/scripts/mtr_bootstrap.cnf
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2571-2623
Timestamp: 2025-11-07T07:10:40.633Z
Learning: Repo: eloqdata/eloqsql PR: 154
File/Area: storage/eloq/ha_eloq.cc (eloq_init_func cluster membership resolution)
Learning: In this repo’s ng_config, the local node is guaranteed to be a candidate (NodeConfig::is_candidate_ == true). Therefore, assigning native_ng_id only when is_candidate_ is true is acceptable; the key improvement is to break early from both loops once the local node is found.
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.
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.
📚 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/pr.ent.bash
📚 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/pr.ent.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/pr.ent.bash
🧬 Code graph analysis (1)
concourse/scripts/pr.ent.bash (2)
concourse/scripts/main.ent.bash (1)
update_config_template(49-69)concourse/scripts/main.bash (1)
update_config_template(44-64)
🔇 Additional comments (2)
concourse/scripts/pr.ent.bash (2)
69-89: Config template updates look consistent with eloq_store/MinIO migration.
The added substitutions for eloq_store keys and txlog URL composition are aligned with the new config model.
139-155: ELOQDSS_ELOQSTORE CMake flag looks right.
The top-level CMake configuration now aligns with the new backend selection.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
2 participants
Summary by CodeRabbit
Chores
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.