TRT-2801: Rewrite bug loader to use COPY + bulk SQL operations#3784
Open
mstaeble wants to merge 1 commit into
Open
TRT-2801: Rewrite bug loader to use COPY + bulk SQL operations#3784mstaeble wants to merge 1 commit into
mstaeble wants to merge 1 commit into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
WalkthroughThe bug loader now fetches bug and association data from BigQuery, stages it in PostgreSQL temporary tables, and performs transactional upserts and reconciliation. BigQuery initialization errors now wrap the correct error, and bug-loader query labels were renamed. ChangesBug loader pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SippyLoader
participant BugLoader
participant BigQuery
participant PostgreSQL
SippyLoader->>BugLoader: initialize with context and clients
BugLoader->>BigQuery: fetch bugs and test/job associations
BigQuery-->>BugLoader: return query rows
BugLoader->>PostgreSQL: copy rows into temporary tables
BugLoader->>PostgreSQL: upsert bugs and synchronize associations
BugLoader->>PostgreSQL: reconcile triages
PostgreSQL-->>BugLoader: commit transaction
Suggested labels: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/dataloader/bugloader/bugloader.go`:
- Around line 246-306: Add end-to-end regression tests for BugLoader.loadIntoDB
covering transaction rollback, empty desired sets, soft-deleted bugs, and
association/triage reconciliation. Also add tests for the transaction and
direct-connection cleanup helpers in pkg/db/pgx.go, including correct handling
of pgx.ErrTxClosed; update both affected files as required and ensure the
lifecycle behavior is exercised through realistic database operations.
- Around line 377-383: Update both deletion statements around deleteTag and the
corresponding job-association deletion so their scope includes every fetched
bug, not only bug IDs present in the temporary association tables. Use the
fetched-bug identity set as the deletion driver, then retain the existing NOT
EXISTS checks against the desired associations so stale rows are removed when a
bug’s desired set is empty.
- Around line 326-349: Update the desiredBugTests and desiredBugJobs association
queries to join only bugs where b.deleted_at IS NULL, preventing soft-deleted
bugs from receiving new association rows. Preserve the existing association
synchronization behavior for active bugs.
🪄 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: 23f86f8e-d836-4918-aed6-def73b7c85e3
📒 Files selected for processing (5)
cmd/sippy/load.gopkg/bigquery/bqlabel/labels.gopkg/dataloader/bugloader/bugloader.gopkg/dataloader/bugloader/bugloader_test.gopkg/db/pgx.go
💤 Files with no reviewable changes (1)
- pkg/dataloader/bugloader/bugloader_test.go
mstaeble
force-pushed
the
worktree-bug-loader-copy
branch
from
July 16, 2026 19:51
632cbee to
8c29579
Compare
mstaeble
marked this pull request as ready for review
July 16, 2026 20:13
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
@coderabbitai resolve |
Contributor
✅ Action performedComments resolved and changes approved. |
openshift-ci
Bot
added
the
ready-for-human-review
mstaeble
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@mstaeble: This pull request references TRT-2800 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
mstaeble
changed the title
|
@mstaeble: This pull request references TRT-2801 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. |
Replace per-row GORM operations (~4000 individual DB round-trips) with PostgreSQL COPY protocol and bulk SQL. Use three separate BigQuery queries (matching the prod query shapes) for independent slot allocation and simpler execution plans: 1. All bugs in the recency window (metadata only, no joins) 2. Test/component associations (jira_id, link_name pairs) 3. Job associations (jira_id, link_name pairs) Each query result loads into its own temp table via COPY protocol, with zero application-level data merging. Key changes: - Three BQ queries with dedicated temp tables (tmp_bugs, tmp_test_assocs, tmp_job_assocs) instead of one combined query - Bugs upserted from tmp_bugs filtered by existence in association tables or matching triage URL, using INSERT...ON CONFLICT with IS DISTINCT FROM to skip no-op updates - Test and job associations synced via anti-join DELETE + INSERT ON CONFLICT DO NOTHING, with UniqueID expansion handled by SQL joins on test_ownerships instead of Go-side map lookups - Triage reconciliation (description sync, bug linking, auto-resolve) done as bulk UPDATEs instead of per-row GORM Save calls - All operations wrapped in a single transaction for atomicity - Generic fetchFromBQ[T] function with transform callback for type-safe BQ iteration and row validation - Generic CopyToTempTable helper in pkg/db/pgx.go with ON COMMIT DROP for transaction-scoped usage and deferred cleanup for bare connections - Shared ticketRecencyFilter constant eliminates WHERE clause duplication between allBugsQuery and ticketCTE - Association queries guarded with INNER JOIN bugs to prevent FK violations from BQ timing skew Note: triage updates no longer go through GORM hooks, so audit log entries are not created for automated bug loader operations (description sync, bug linking, auto-resolve). This is intentional; the updated_at column is set explicitly in each UPDATE statement. Also fixes pre-existing bug in load.go where errors.WithMessage wrapped the wrong variable (err instead of bigqueryErr), silently swallowing BigQuery client creation errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mstaeble
force-pushed
the
worktree-bug-loader-copy
branch
from
July 17, 2026 17:58
8c29579 to
e4b85b9
Compare
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
|
Scheduling required tests: |
Contributor
|
/lgtm |
openshift-ci
Bot
added
the
do-not-merge/hold
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mstaeble, 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
Contributor
Author
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
Contributor
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
Contributor
|
@mstaeble: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pgx.CopyFrom) and bulk SQL operations, replacing the old GORM-based row-by-row approachCopyToTempTable[T]helper inpkg/db/pgx.gothat handles temp table creation, COPY loading, and cleanup (withON COMMIT DROPfor transaction-scoped tables)jira_id), reporting errors throughbl.addErrorfor observabilitycmd/sippy/load.gowhereerrors.WithMessagewrapped the wrong variable (errinstead ofbigqueryErr)Notable design decisions
AfterUpdatehooks createdAuditLogentries for triage changes. These are intentionally omitted since the bug loader is an automated process running on a schedule, and audit logs for automated bulk operations add noise without value.INNER JOIN bugsto guard against timing skew where BQ returns association rows referencing bugs that don't pass the upsert filter. Self-healing on next run either way.deleted_at IS NULL, replicating what GORM did automatically.tmp_bugs), not just bugs with current associations, so stale rows are cleaned up when a bug loses all its matches (matching the old GORMReplace()behavior).Performance
Production timing from the latest
fetchdatapod logs. Staging timing from local testing. Hardware differs, but the bulk SQL operations are the main win: under 1 second vs ~28 seconds for GORM's row-by-row upsert of 1,438 bugs.Behavioral parity
Verified against staging database:
Test plan
make lintpassesmake testpassesmake e2epasses🤖 Generated with Claude Code