Remove DynamoDB and Bigtable storage backends - #551
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change removes DynamoDB and Bigtable from CMake data-store selection, updates RocksDB catalog handling, serializes Cassandra schema images during catalog operations, changes build flags, refreshes the data substrate revision, and updates related documentation. ChangesStorage configuration and catalog integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CMakeLists.txt`:
- Around line 168-170: Complete RocksDB schema-image support before enabling the
ROCKSDB build definition: update the prebuilt table registration path using
GenKvTableName to generate and register the expected RocksDB catalog metadata
image, and replace the TODO(lokax) ImageDataStore branches in catalog reads and
writes with the corresponding schema-image handling. Only expose
DATA_STORE_TYPE_ROCKSDB after these paths produce and consume valid, consistent
catalog metadata.
In `@data_substrate`:
- Line 1: Update the data_substrate gitlink to the final reachable commit on the
eloqdata/tx_service main branch after the submodule PR is squash-merged,
replacing the outdated parent reference while following the repository’s
submodule update policy.
In `@README.md`:
- Line 175: Move the architecture paragraph describing TxService, Log Service,
and RocksDB/EloqStore storage outside the surrounding HTML comment in README.md
so it renders as visible user-facing documentation; preserve the paragraph text
and surrounding documentation structure.
In `@src/eloqkv_catalog_factory.cpp`:
- Line 64: Update the catalog-info construction in the RocksDB path to use the
RocksDB-specific concrete KVCatalogInfo type, ensuring its declaration is
included and the type is properly qualified or imported within namespace EloqKV.
Preserve the existing kv_info_ ownership and initialization behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f24dcd5-a580-4aca-8eee-bf911f87c21d
📒 Files selected for processing (7)
.github/scripts/common.shCLAUDE.mdCMakeLists.txtREADME.mddata_substratesrc/eloqkv_catalog_factory.cppsrc/redis_service.cpp
💤 Files with no reviewable changes (2)
- .github/scripts/common.sh
- src/redis_service.cpp
Context
DynamoDB and Bigtable are no longer supported storage backends, but EloqKV still advertised both
WITH_DATA_STOREvalues and retained backend-specific catalog branches. The enterprise build helper also passed the already-removedOPEN_LOG_SERVICEoption.This PR intentionally points
data_substrateat the companion PR branch commit so EloqKV CI validates the combined change:data_substratecommit:550fe3db8495b060f2e2a74bbf756be620e4ad6aBehavior before and after
Before, top-level CMake advertised and accepted
DYNAMODBandBIGTABLE;OPEN_LOG_SERVICEwas passed despite having no consumer.After, the supported
WITH_DATA_STOREvalues areROCKSDB,ELOQDSS_ROCKSDB,ELOQDSS_ROCKSDB_CLOUD_S3,ELOQDSS_ROCKSDB_CLOUD_GCS, andELOQDSS_ELOQSTORE. Removed values fail explicitly withUnknown WITH_DATA_STORE.Implementation
OPEN_LOG_SERVICEargument from.github/scripts/common.sh.data_substratepointer from82cea28to the companion PR commit550fe3d.Design decisions and alternatives
The parent PR carries the unmerged companion commit instead of waiting for the submodule PR to land, allowing the full EloqKV CI matrix to exercise both repositories together. The parent pointer must be refreshed to the merged
data_substrate/maincommit before final merge if the companion PR is squash-merged to a different SHA.Test plan
Commands and results:
clang-format-18was unavailable in the workspace. No runtime/TCL tests were run locally; this PR carries the companion branch commit specifically to run the repository CI matrix.Risk assessment
This intentionally breaks configurations that still select DynamoDB or Bigtable. Supported RocksDB and EloqStore builds passed locally. The principal integration risk is the temporary submodule pointer to an unmerged commit; reviewers should verify it is replaced with the final reachable
data_substrate/maincommit before merging EloqKV.Rollback plan
Revert this PR and the companion
data_substratePR.Reviewer guide
Review the top-level
CMakeLists.txt,src/eloqkv_catalog_factory.cpp, and thedata_substratepointer first. The full backend implementation deletion is in tx_service PR #536.Follow-up work
After tx_service PR #536 is merged, update this PR's submodule pointer to the resulting commit on
data_substrate/mainand rerun CI before merge.Summary by CodeRabbit