-
Notifications
You must be signed in to change notification settings - Fork 131
feat(engine): add RocksDB engine run script #3220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
commit: |
3723de7 to
c504886
Compare
ce8a0e9 to
58fd00a
Compare
Pull Request ReviewSummaryThis PR adds a new run script for the engine using RocksDB (FileSystem database backend). The script follows the basic pattern of existing run scripts but has some issues that should be addressed. Issues Found1. Missing Database ConfigurationSeverity: High The script doesn't explicitly configure the database backend. While RocksDB is the default (via FileSystem), this should be made explicit for clarity and to prevent confusion if defaults change. Recommendation: Add explicit configuration similar to how engine-postgres.sh sets RIVET__POSTGRES__URL, or add a comment explaining the default behavior. 2. Inconsistent Naming ConventionSeverity: Low The script is named engine-rocksdb.sh but the actual database backend in the config is called FileSystem. While RocksDB is the underlying technology, the config enum is Database::FileSystem. Recommendation: Consider renaming to engine-filesystem.sh for consistency with the config, or add a comment explaining the naming. 3. No Validation or Prerequisites CheckSeverity: Medium Unlike engine-postgres.sh which checks if Postgres is reachable and provides helpful error messages, this script has no validation. Recommendation: Consider adding basic validation to ensure the data directory can be created. Positive Aspects
RecommendationRequest Changes - The missing database configuration should be addressed to make the script's behavior explicit and prevent future confusion. |
Code ReviewI've reviewed this PR and have the following feedback: SummaryThis PR adds a new script scripts/run/engine-rocksdb.sh to run the Rivet engine with RocksDB as the database backend. Critical Issue: Missing Environment ConfigurationThe script doesn't configure the database backend at all. Looking at the codebase, the engine uses rivet_config::Config which defaults to Database::FileSystem (which uses RocksDB internally). The script name suggests it's specifically for RocksDB, but it doesn't actually configure anything database-specific. Since the default is already FileSystem/RocksDB, the script works, but it's confusing because it relies on implicit defaults rather than explicit configuration. Recommendation: Either rename to engine.sh or engine-local.sh, or add a comment explaining that RocksDB is the default. Medium Issue: Missing Health ChecksUnlike engine-postgres.sh which includes checks for nc command availability, verifies Postgres is reachable, and provides helpful error messages, this script has no validation for the data directory writability or disk space. Recommendation: Add basic validation for data directory creation and writability. Minor Issue: Naming ConsistencyThe config uses Database::FileSystem (not Database::RocksDB) - RocksDB is an implementation detail. Consider renaming to engine-filesystem.sh or engine-local.sh to match the config enum naming (see packages/common/config/src/config/db.rs:10-13). Positive Aspects
RecommendationAt minimum, add a comment explaining that this uses the default FileSystem/RocksDB backend. Ideally, add validation checks similar to the Postgres script and consider renaming for clarity. |

No description provided.