feat(sis): Adding deployment_id and gpu_spec_id and index for them to instances table - #602
feat(sis): Adding deployment_id and gpu_spec_id and index for them to instances table#602dmikhaylovnv wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe Cassandra schema adds nullable deployment and GPU specification UUID columns to ChangesInstance lookup schema
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@migrations/cassandra/keyspaces/sis_api/07_add_deployment_and_gpu_spec_id.up.sql`:
- Line 2: Extend migrations/cassandra/tests/test-execute-sqls.sh to cover
migrations 07 and 08: verify both UUID columns, both indexes, migration
ordering, and successful repeat application; the migration SQL files require no
direct changes. If coverage is intentionally omitted, document the reason in the
Pull Request description.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 82eab019-e4ef-4424-b25f-8a2d7e6adcc1
📒 Files selected for processing (2)
migrations/cassandra/keyspaces/sis_api/07_add_deployment_and_gpu_spec_id.up.sqlmigrations/cassandra/keyspaces/sis_api/08_add_indexes_for_deployment_and_gpu_spec_id.up.sql
| @@ -0,0 +1,2 @@ | |||
| -- Add deployment_id and gpu_specification_id UUID to instances table | |||
| ALTER TABLE sis_api.instances ADD IF NOT EXISTS (deployment_id uuid, gpu_specification_id uuid); | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- migration files ---'
git ls-files 'migrations/cassandra/keyspaces/sis_api/*' | sort
printf '%s\n' '--- repository test and migration references ---'
rg -n -i --glob '!vendor' --glob '!node_modules' \
'cassandra|migration|migrations|IF NOT EXISTS|StorageAttachedIndex' \
.github Makefile* README* go.mod go.sum package.json pyproject.toml 2>/dev/null | head -n 250
printf '%s\n' '--- likely test files ---'
git ls-files | rg -i '(^|/)(test|tests|spec|specs)(/|$)|(_test|\.spec|\.test)\.' | head -n 250Repository: NVIDIA/nvcf
Length of output: 17899
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- migration test script ---'
wc -l migrations/cassandra/tests/test-execute-sqls.sh
cat -n migrations/cassandra/tests/test-execute-sqls.sh
printf '%s\n' '--- Cassandra migration tree ---'
git ls-files 'migrations/cassandra/**' | sort
printf '%s\n' '--- relevant migration contents ---'
for f in \
migrations/cassandra/keyspaces/sis_api/01_init_keyspace.up.sql \
migrations/cassandra/keyspaces/sis_api/03_init_tables.up.sql \
migrations/cassandra/keyspaces/sis_api/07_add_deployment_and_gpu_spec_id.up.sql \
migrations/cassandra/keyspaces/sis_api/08_add_indexes_for_deployment_and_gpu_spec_id.up.sql
do
echo "### $f"
cat -n "$f"
done
printf '%s\n' '--- tracked change summary ---'
git diff --stat
git status --shortRepository: NVIDIA/nvcf
Length of output: 31709
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- migration documentation ---'
for f in migrations/cassandra/README.md migrations/cassandra/keyspaces/README.md; do
echo "### $f"
cat -n "$f"
done
printf '%s\n' '--- migration test references ---'
rg -n -C 4 \
'test-execute-sqls|migrations/cassandra/tests|execute_sqls\.sh|migrations/cassandra' \
.github Makefile* BUILD* WORKSPACE* WORKSPACE.bazel 2>/dev/null | head -n 300
printf '%s\n' '--- migration executor ---'
wc -l migrations/cassandra/execute_sqls.sh
cat -n migrations/cassandra/execute_sqls.sh
printf '%s\n' '--- migration ordering and idempotency markers ---'
python3 - <<'PY'
from pathlib import Path
root = Path("migrations/cassandra/keyspaces")
for keyspace in sorted(p.parent.name for p in root.glob("*/01_init_keyspace.up.sql")):
files = sorted(p.name for p in (root / keyspace).glob("*.up.sql"))
print(keyspace, ":", ", ".join(files))
for name in files:
text = (root / keyspace / name).read_text()
print(f" {name}: IF NOT EXISTS={text.upper().count('IF NOT EXISTS')}")
PYRepository: NVIDIA/nvcf
Length of output: 22912
Add coverage for the new Cassandra migrations.
Extend migrations/cassandra/tests/test-execute-sqls.sh to check both UUID columns, both indexes, migration ordering, and repeat application. If tests are intentionally omitted, document the reason in the Pull Request description.
📍 Affects 2 files
migrations/cassandra/keyspaces/sis_api/07_add_deployment_and_gpu_spec_id.up.sql#L2-L2(this comment)migrations/cassandra/keyspaces/sis_api/08_add_indexes_for_deployment_and_gpu_spec_id.up.sql#L2-L3
🤖 Prompt for 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.
In
`@migrations/cassandra/keyspaces/sis_api/07_add_deployment_and_gpu_spec_id.up.sql`
at line 2, Extend migrations/cassandra/tests/test-execute-sqls.sh to cover
migrations 07 and 08: verify both UUID columns, both indexes, migration
ordering, and successful repeat application; the migration SQL files require no
direct changes. If coverage is intentionally omitted, document the reason in the
Pull Request description.
Source: Coding guidelines
TL;DR
Adding deployment_id and gpu_spec_id and their indexes for them to instances table
Issues
Resolves NVIDIA/nvcf#11152
Summary by CodeRabbit