TRT-1989: model changes to prep for partitioning#3532
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@neisw: This pull request references TRT-1989 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThis PR extends Prow job run and test record models with partitioning fields ( ChangesProw run and test partitioning fields
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 15 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neisw The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![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
🤖 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 `@pkg/db/models/prow.go`:
- Around line 92-99: ProwJobID was added as a denormalized lookup key but lacks
an index; modify the struct field declaration for ProwJobID (the ProwJobID field
in the prow job run tests model) to add a GORM index tag (e.g., include
`gorm:"index"` or a named index) so queries filtering by ProwJobID use an index
rather than causing full table scans.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d96f992c-5f51-4c25-a083-71ee5bfd7ce3
📒 Files selected for processing (5)
cmd/sippy/seed_data.gopkg/api/job_runs_test.gopkg/dataloader/prowloader/prow.gopkg/db/models/prow.gopkg/sippyserver/pr_new_tests_worker_test.go
| // used for variants | ||
| // skips joining on ProwJobRunID just to get ProwJobID | ||
| ProwJobID uint | ||
| // used for partitioning | ||
| ProwJobRunTimestamp time.Time `gorm:"expression:DATE(timestamp AT TIME ZONE 'UTC')"` | ||
| // used for partitioning | ||
| ProwJobRunRelease string `gorm:"varchar(10)"` | ||
| TestID uint `gorm:"index;index:idx_prow_job_run_tests_test_id_status"` |
There was a problem hiding this comment.
Index the new denormalized lookup key.
ProwJobID is added to avoid a join, but without an index this can still force full scans on prow_job_run_tests under real data volume.
💡 Suggested fix
- ProwJobID uint
+ ProwJobID uint `gorm:"index"`📝 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.
| // used for variants | |
| // skips joining on ProwJobRunID just to get ProwJobID | |
| ProwJobID uint | |
| // used for partitioning | |
| ProwJobRunTimestamp time.Time `gorm:"expression:DATE(timestamp AT TIME ZONE 'UTC')"` | |
| // used for partitioning | |
| ProwJobRunRelease string `gorm:"varchar(10)"` | |
| TestID uint `gorm:"index;index:idx_prow_job_run_tests_test_id_status"` | |
| // used for variants | |
| // skips joining on ProwJobRunID just to get ProwJobID | |
| ProwJobID uint `gorm:"index"` | |
| // used for partitioning | |
| ProwJobRunTimestamp time.Time `gorm:"expression:DATE(timestamp AT TIME ZONE 'UTC')"` | |
| // used for partitioning | |
| ProwJobRunRelease string `gorm:"varchar(10)"` | |
| TestID uint `gorm:"index;index:idx_prow_job_run_tests_test_id_status"` |
🤖 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 `@pkg/db/models/prow.go` around lines 92 - 99, ProwJobID was added as a
denormalized lookup key but lacks an index; modify the struct field declaration
for ProwJobID (the ProwJobID field in the prow job run tests model) to add a
GORM index tag (e.g., include `gorm:"index"` or a named index) so queries
filtering by ProwJobID use an index rather than causing full table scans.
There was a problem hiding this comment.
@coderabbitai, we are only staging the new columns for now and beginning data collection. We will be migrating the tables and backfilling the new columns at a later time. Indexing will be updated then.
There was a problem hiding this comment.
@neisw, understood — thanks for the context! A phased approach (stage columns → collect data → migrate/backfill → add indexes) makes sense here. Would you like me to open a follow-up GitHub issue to track adding the index on ProwJobID (and any other partitioning-related indexes) once the migration and backfill are complete?
✏️ Learnings added
Learnt from: neisw
Repo: openshift/sippy PR: 3532
File: pkg/db/models/prow.go:92-99
Timestamp: 2026-05-15T13:46:44.315Z
Learning: In `pkg/db/models/prow.go`, the `ProwJobID`, `ProwJobRunTimestamp`, and `ProwJobRunRelease` fields added to `ProwJobRunTest`, as well as `ProwJobRelease` on `ProwJobRun` and the timestamp/release fields on `ProwJobRunTestOutput`, are being staged in a phased approach: columns are added first for data collection, with table migration, backfill, and index creation deferred to a later PR. Do not flag missing indexes on these new columns as blocking issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Scheduling required tests: |
|
@neisw: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Model changes include
Summary by CodeRabbit