TRT-2848: Refresh summary tables incrementally during prow load - #3852
TRT-2848: Refresh summary tables incrementally during prow load#3852mstaeble wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@mstaeble: This pull request references TRT-2848 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
|
Skipping CI for Draft Pull Request. |
WalkthroughProw ingestion now uses shared ChangesProw persistence and summary maintenance
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 17 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (17 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 |
mstaeble
changed the title
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: 5
🧹 Nitpick comments (3)
test/integration/pgwriter_test.go (2)
18-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive these status values from the canonical test-status constants.
Hardcoded
1/12/13will silently drift if the junit status enum changes. Prefer referencing the existing status constants used by the loader (e.g. thev1test status type) rather than redeclaring literals here.#!/bin/bash # Locate canonical test status constants for 1 (success), 12 (failure), 13 (flake) rg -nP --type=go -C2 '=\s*(1|12|13)\b' -g '**/apis/**' | rg -i -C2 'status|flake|fail' ast-grep run --pattern 'const ($$$)' --lang go pkg/apis | head -80🤖 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 `@test/integration/pgwriter_test.go` around lines 18 - 22, Replace the literal values in the statusSuccess, statusFailure, and statusFlake declarations with the canonical test-status constants from the existing v1 status type used by the loader, preserving the current success, failure, and flake mappings.
603-641: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for carry-forward across multiple releases.
Every
carryForwardcall passes a single release, so the new parallel per-release path is never exercised with >1 release. Adding[]string{"4.18", "4.17"}to one case would cover the concurrent path and its error aggregation.🤖 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 `@test/integration/pgwriter_test.go` around lines 603 - 641, The carryForward tests only exercise a single release and miss the multi-release parallel path. Update an existing carryForward test, such as TestCarryForwardIsReleaseScoped, to pass both “4.18” and “4.17” together, and assert each release’s expected result so concurrent processing and error aggregation are covered.pkg/sippyserver/server.go (1)
150-151: 🩺 Stability & Availability | 🔵 TrivialDropping
sippy_cumulative_summary_refresh_millisbreaks any dashboard/alert on it.The incremental path in
pgwriterhas no equivalent timing metric, so summary-maintenance latency becomes unobservable. Consider emitting a comparable histogram from the writer/carry-forward path and retiring the old metric in dashboards.🤖 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/sippyserver/server.go` around lines 150 - 151, Preserve observability for summary-maintenance latency when removing sippy_cumulative_summary_refresh_millis by adding a comparable timing histogram to the pgwriter incremental/carry-forward path. Instrument the relevant writer flow, using the existing metric naming and labeling conventions where applicable, and ensure dashboards and alerts are migrated to the replacement metric rather than left referencing the retired one.
🤖 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/prowloader/pgwriter/pgwriter.go`:
- Around line 589-612: Update carryForwardRelease and the related handling
around findLatestDateWithData so a release with cumulative data only older than
the 30-day lookback is treated as a skipped carry-forward: log the
stale/no-recent-data condition and return nil, rather than propagating an error
that aborts Load. Preserve error propagation for genuine database failures and
keep normal carry-forward behavior unchanged when a recent date is found.
- Around line 472-491: Update ensureDailyTotalRows to convert the civil.Date day
argument into a PostgreSQL-compatible bind value before passing it to tx.Exec,
using the existing date-binding convention such as a UTC date string or
time.Time. Use the converted value consistently for both date placeholders while
preserving the current insert and error-handling behavior.
- Around line 398-417: Extend the cumulative-summary day-loop upper bound in the
release-date processing flow to the maximum of tomorrow and the latest batch
date from releaseDates. Preserve the existing minimum-date grouping and
ensureCumulativeSummaryRows/updateCumulativeSummaries calls, so future-dated
batches are included rather than dropped.
In `@test/integration/pgwriter_test.go`:
- Around line 222-224: Replace the vacuous soft-delete assertions with validity
checks: in test/integration/pgwriter_test.go lines 222-224 for the models.Test
row and lines 1039-1041 for the models.Suite row, use require.True on
softDeleted.DeletedAt.Valid with an appropriate failure message. Preserve the
existing unscoped queries.
- Around line 696-699: In the CarryForwardCumulativeSummaries test, replace
assert.Error with require.Error before calling err.Error() in the subsequent
assertion, so a nil error stops the test cleanly before dereferencing it.
---
Nitpick comments:
In `@pkg/sippyserver/server.go`:
- Around line 150-151: Preserve observability for summary-maintenance latency
when removing sippy_cumulative_summary_refresh_millis by adding a comparable
timing histogram to the pgwriter incremental/carry-forward path. Instrument the
relevant writer flow, using the existing metric naming and labeling conventions
where applicable, and ensure dashboards and alerts are migrated to the
replacement metric rather than left referencing the retired one.
In `@test/integration/pgwriter_test.go`:
- Around line 18-22: Replace the literal values in the statusSuccess,
statusFailure, and statusFlake declarations with the canonical test-status
constants from the existing v1 status type used by the loader, preserving the
current success, failure, and flake mappings.
- Around line 603-641: The carryForward tests only exercise a single release and
miss the multi-release parallel path. Update an existing carryForward test, such
as TestCarryForwardIsReleaseScoped, to pass both “4.18” and “4.17” together, and
assert each release’s expected result so concurrent processing and error
aggregation are covered.
🪄 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: Pro Plus
Run ID: b2e681e0-a6cb-4e7b-8ad1-c02fe7166add
📒 Files selected for processing (6)
pkg/dataloader/prowloader/accumulate_test.gopkg/dataloader/prowloader/pgwriter/pgwriter.gopkg/dataloader/prowloader/prow.gopkg/sippyserver/server.gotest/integration/pgwriter_test.gotest/integration/util/schema.go
33eeaf7 to
f52bdb3
Compare
openshift-ci
Bot
added
the
ready-for-human-review
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
🧹 Nitpick comments (7)
pkg/dataloader/prowloader/pgwriter/pgwriter.go (4)
457-478: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
defer rows.Close()over manual close on each path.Two explicit
rows.Close()calls are easy to miss when a new early return is added.defer rows.Close()right after the error check is the idiomatic form and remains correct with therows.Err()check.🤖 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/dataloader/prowloader/pgwriter/pgwriter.go` around lines 457 - 478, Update queryReleaseDates to defer rows.Close() immediately after the successful tx.Query check, then remove both explicit rows.Close() calls while preserving the existing scan-error and rows.Err() handling.
437-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded status codes will silently drift from the Go constants.
1,12,13aresippyprocessingv1.TestStatusSuccess/Failure/Flake. Nothing ties the SQL to those constants, so a renumbering miscounts summaries with no compile error. Since these are internal constants (no injection concern), bind them as parameters or at minimum add a comment naming each.♻️ Sketch
- COUNT(*) FILTER (WHERE tmp.status = 1) AS successes, - COUNT(*) FILTER (WHERE tmp.status = 12) AS failures, - COUNT(*) FILTER (WHERE tmp.status = 13) AS flakes, + COUNT(*) FILTER (WHERE tmp.status = $1) AS successes, + COUNT(*) FILTER (WHERE tmp.status = $2) AS failures, + COUNT(*) FILTER (WHERE tmp.status = $3) AS flakes,with
int(sippyprocessingv1.TestStatusSuccess), int(sippyprocessingv1.TestStatusFailure), int(sippyprocessingv1.TestStatusFlake)passed toExec(themin/max_*_tsfilters need the same treatment).🤖 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/dataloader/prowloader/pgwriter/pgwriter.go` around lines 437 - 439, Update the summary SQL in the pgwriter query to replace hardcoded status values with parameters bound from sippyprocessingv1.TestStatusSuccess, TestStatusFailure, and TestStatusFlake, and apply the same parameterization to the min/max timestamp filters. Ensure the corresponding Exec arguments and placeholder ordering remain aligned.
407-425: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueTwo day-at-a-time
Execloops overtest_cumulative_summaries. Both the in-batch cumulative upsert and the carry-forward catch-up iterate one day at a time, issuing a separate statement per day per release. The shared fix is to drive each from a single set-based statement overgenerate_series(<start>, <end>, '1 day'), which cuts round trips and makes multi-day work atomic.
pkg/dataloader/prowloader/pgwriter/pgwriter.go#L407-L425: replace thefor day := minDate; !day.After(tomorrow)loop's per-dayensureCumulativeSummaryRows/updateCumulativeSummariescalls with day-set-joined statements so a back-dated batch doesn't multiply round trips inside the write transaction.pkg/dataloader/prowloader/pgwriter/pgwriter.go#L622-L640: replace the per-day carry-forwardINSERTwith onegenerate_series-driven insert (or wrap the loop in a single transaction) so a multi-day catch-up can't leave the table partially advanced.🤖 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/dataloader/prowloader/pgwriter/pgwriter.go` around lines 407 - 425, Replace the per-day loop at pkg/dataloader/prowloader/pgwriter/pgwriter.go:407-425 with set-based statements using generate_series from each release’s minimum date through tomorrow, preserving the ensureCumulativeSummaryRows and updateCumulativeSummaries behavior in the joined day set. Also replace the carry-forward per-day INSERT at pkg/dataloader/prowloader/pgwriter/pgwriter.go:622-640 with one generate_series-driven statement, or make the loop a single transaction, so both multi-day paths avoid per-day round trips and remain atomic.
480-499: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
NOT EXISTS+INSERTis a check-then-act;ON CONFLICT DO NOTHINGis safer and self-documenting.Both
ensure*Rowshelpers rely onNOT EXISTSto avoid duplicates. That's fine under today's single-writer design (Writeis driven serially from one goroutine), but it silently degrades to duplicate rows or a constraint error if a second loader ever writes concurrently.ON CONFLICT (…) DO NOTHINGon the natural key is both race-safe and cheaper.Separately,
ensureCumulativeSummaryRowsneeds theGROUP BY(becausebatch_date <= $1spans days) whileensureDailyTotalRowsdoesn't (batch_deltasis already unique per release+date). A one-line "why" comment on the cumulative variant would save the next reader that deduction.Also applies to: 525-545
🤖 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/dataloader/prowloader/pgwriter/pgwriter.go` around lines 480 - 499, Update ensureDailyTotalRows and ensureCumulativeSummaryRows to use INSERT ... ON CONFLICT DO NOTHING on each table’s natural key instead of NOT EXISTS checks, preserving the existing inserted values and aggregation behavior. Add a brief comment in ensureCumulativeSummaryRows explaining that GROUP BY is required because batch_date <= the requested date spans multiple days; keep ensureDailyTotalRows ungrouped because its source is already unique per release and date.test/integration/pgwriter_test.go (2)
721-742: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name covers only half of what it asserts.
TestCarryForwardErrorsWhenNoDataWithinLookbackfirst asserts the no-op path when no data exists at all (line 725), then the error path beyond the 30-day window. Splitting these into two tests (e.g.TestCarryForwardIsNoOpWhenNoDataExists+ the current name) makes a failure immediately attributable.🤖 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 `@test/integration/pgwriter_test.go` around lines 721 - 742, Split TestCarryForwardErrorsWhenNoDataWithinLookback into two focused tests: move the initial no-data assertion into a new TestCarryForwardIsNoOpWhenNoDataExists, and keep the seeded old-data scenario and far-future lookback error assertion in TestCarryForwardErrorsWhenNoDataWithinLookback.
830-833: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the job-run and test rows rolled back too, not just daily totals.
insertJobRuns/insertTestResultsrun before the future-date guard fires, so they're the rows most at risk if the rollback ever regressed. Checking onlytest_daily_totals(which would be empty even without a rollback, since the guard precedes the summary writes) under-tests the invariant.💚 Proposed additional assertions
var dtCount int64 require.NoError(t, dbc.DB.Model(&models.TestDailyTotal{}).Count(&dtCount).Error) assert.Equal(t, int64(0), dtCount, "transaction should have rolled back, no daily totals written") + + var runCount int64 + require.NoError(t, dbc.DB.Model(&models.ProwJobRun{}).Count(&runCount).Error) + assert.Equal(t, int64(0), runCount, "transaction should have rolled back, no job runs written") + + var jrtCount int64 + require.NoError(t, dbc.DB.Model(&models.ProwJobRunTest{}).Count(&jrtCount).Error) + assert.Equal(t, int64(0), jrtCount, "transaction should have rolled back, no test results written")🤖 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 `@test/integration/pgwriter_test.go` around lines 830 - 833, Extend the rollback assertions in this integration test to also count rows from the models written by insertJobRuns and insertTestResults, and assert both counts are zero after the future-date guard failure. Keep the existing daily-total assertion and use the corresponding model symbols to verify all transaction writes were rolled back.pkg/dataloader/prowloader/accumulate_test.go (1)
36-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
callsis captured and copied but never asserted in the table test.Every case's
writerFuncdeep-copies each batch intocalls, yet the subtest only asserts onpl.errors. Either assert the expected batch count/sizes (which is what the copies were evidently written for, and would pin the 100-row flush threshold) or drop thecallsplumbing from this table to cut the noise.TestAccumulateAndWriteJobRuns_PerBatchErrorsdoes assertLen(calls, 4), so the pattern is clearly intended.Also applies to: 124-125, 149-149
🤖 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/dataloader/prowloader/accumulate_test.go` around lines 36 - 51, Update the table-driven test around its writerFunc and subtest assertions to validate the captured calls, including expected batch count and sizes that confirm the 100-row flush threshold; apply the same assertion update to the additional cases noted in the comment. Reuse the existing calls capture and match the pattern from TestAccumulateAndWriteJobRuns_PerBatchErrors rather than removing the plumbing.Source: Path instructions
🤖 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.
Nitpick comments:
In `@pkg/dataloader/prowloader/accumulate_test.go`:
- Around line 36-51: Update the table-driven test around its writerFunc and
subtest assertions to validate the captured calls, including expected batch
count and sizes that confirm the 100-row flush threshold; apply the same
assertion update to the additional cases noted in the comment. Reuse the
existing calls capture and match the pattern from
TestAccumulateAndWriteJobRuns_PerBatchErrors rather than removing the plumbing.
In `@pkg/dataloader/prowloader/pgwriter/pgwriter.go`:
- Around line 457-478: Update queryReleaseDates to defer rows.Close()
immediately after the successful tx.Query check, then remove both explicit
rows.Close() calls while preserving the existing scan-error and rows.Err()
handling.
- Around line 437-439: Update the summary SQL in the pgwriter query to replace
hardcoded status values with parameters bound from
sippyprocessingv1.TestStatusSuccess, TestStatusFailure, and TestStatusFlake, and
apply the same parameterization to the min/max timestamp filters. Ensure the
corresponding Exec arguments and placeholder ordering remain aligned.
- Around line 407-425: Replace the per-day loop at
pkg/dataloader/prowloader/pgwriter/pgwriter.go:407-425 with set-based statements
using generate_series from each release’s minimum date through tomorrow,
preserving the ensureCumulativeSummaryRows and updateCumulativeSummaries
behavior in the joined day set. Also replace the carry-forward per-day INSERT at
pkg/dataloader/prowloader/pgwriter/pgwriter.go:622-640 with one
generate_series-driven statement, or make the loop a single transaction, so both
multi-day paths avoid per-day round trips and remain atomic.
- Around line 480-499: Update ensureDailyTotalRows and
ensureCumulativeSummaryRows to use INSERT ... ON CONFLICT DO NOTHING on each
table’s natural key instead of NOT EXISTS checks, preserving the existing
inserted values and aggregation behavior. Add a brief comment in
ensureCumulativeSummaryRows explaining that GROUP BY is required because
batch_date <= the requested date spans multiple days; keep ensureDailyTotalRows
ungrouped because its source is already unique per release and date.
In `@test/integration/pgwriter_test.go`:
- Around line 721-742: Split TestCarryForwardErrorsWhenNoDataWithinLookback into
two focused tests: move the initial no-data assertion into a new
TestCarryForwardIsNoOpWhenNoDataExists, and keep the seeded old-data scenario
and far-future lookback error assertion in
TestCarryForwardErrorsWhenNoDataWithinLookback.
- Around line 830-833: Extend the rollback assertions in this integration test
to also count rows from the models written by insertJobRuns and
insertTestResults, and assert both counts are zero after the future-date guard
failure. Keep the existing daily-total assertion and use the corresponding model
symbols to verify all transaction writes were rolled back.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c4e232a-ab67-4f99-98ba-eb856ab613e4
📒 Files selected for processing (5)
pkg/dataloader/prowloader/accumulate_test.gopkg/dataloader/prowloader/pgwriter/pgwriter.gopkg/dataloader/prowloader/prow.gopkg/sippyserver/server.gotest/integration/pgwriter_test.go
|
Scheduling required tests: |
f52bdb3 to
3ba7641
Compare
|
Scheduling required tests: |
petr-muller
left a comment
petr-muller
left a comment
There was a problem hiding this comment.
I have reviewed the code and had some convos with Claude Code about and found no problems, but I do not feel too confident about knowing this part of Sippy that well, so LGTM+hold for the case you want someone better with DBs than me to look as well. Unhold as needed
/hold
openshift-ci
Bot
added
the
do-not-merge/hold
| ghCommenter: ghCommenter, | ||
| promPusher: promPusher, | ||
| loadSince: loadSince, | ||
| currentDate: civil.DateOf(time.Now().UTC()), |
There was a problem hiding this comment.
This could be ok for now but what are the implications when we get to loading historical data? There is more to it than just this so it is really just an observation for now, nothing to take action on
There was a problem hiding this comment.
Or maybe this works as any historical updates will need to be brought all the way forward to today.
There was a problem hiding this comment.
When we get to loading historical data, we will want a more deliberate command for that. Thinking aloud, we'll want to load the oldest data first, loading a few days at a time. Then we'll want to separately backfill the summary tables. The commands already exist for that summary backfill that go deliberately day by day.
| // (test_daily_totals, test_cumulative_summaries) are updated | ||
| // incrementally by the prow loader; use "sippy backfill" to repair them. | ||
| func RefreshData(dbc *db.DB, cacheClient cache.Cache, opts RefreshOptions) error { | ||
| log.Infof("Refreshing data") |
There was a problem hiding this comment.
e2e seems to be passing so seed-data may not be an issue. Without new data coming in running the refresh command will just be refreshing the materialized views so all may be fine, posting the review from ai-helpers in this area just to confirm there are no real concerns raised here:
[High] LSP: RefreshData() behavioral change -- server.go: Removing dailysummary.Refresh and cumulativesummary.Refresh from RefreshData() silently changes the function's contract for all
callers, not just the prow loader:
- cmd/sippy/load.go -- calls RefreshData after prow load. Since the prow loader now handles summaries incrementally, this caller gets equivalent behavior. OK.
- cmd/sippy/refresh.go -- explicit "refresh" command. After this PR, sippy refresh will not update daily totals or cumulative summaries. This is a breaking change for operators.
- cmd/sippy/seed_data.go -- seeds data then refreshes. Summary tables will not be rebuilt after seeding.
The PR description mentions "use sippy backfill to repair them," but the refresh and seed-data commands' help text and behavior are not updated to reflect this change. Recommendation:
Either (a) have those commands also invoke backfill/carry-forward for summary tables, or (b) explicitly document the behavioral change in command help text and the PR description.
There was a problem hiding this comment.
The change to "sippy refresh" is deliberate. The backfill command is the tool to use to re-calculate summary tables.
There is probably a gap in the seed_data command. I had not considered that.
There was a problem hiding this comment.
For the seed data path, it has been converted to use the BackfillData function already. It should be functioning properly.
mstaeble
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
3ba7641 to
cefdb9a
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/db/dailysummary/dailysummary_test.go (1)
48-63: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify the complete replacement dispatch contract.
TestBackfill_UsesExplicitDateRangeonly verifies the call count and boundary dates. The test can pass ifbackfillSummariesduplicates one release and skips another release.Assert every expected
(date, release)pair. Add a case whereReplaceRangeForReleasereturns an error and assert thatbackfillSummariesreturns that error.As per coding guidelines, “New or modified functionality should include test coverage.” Based on learnings, non-trivial concurrency behavior requires unit tests.
🤖 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/dailysummary/dailysummary_test.go` around lines 48 - 63, Expand TestBackfill_UsesExplicitDateRange to assert every expected date-and-release pair, ensuring each date in the inclusive range is dispatched exactly once for both releases rather than relying only on count and boundaries. Add a failure case using fakeStore so ReplaceRangeForRelease returns an error, and assert backfillSummaries propagates that error.Sources: Coding guidelines, Learnings
🧹 Nitpick comments (1)
test/integration/pgwriter_test.go (1)
170-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
sets.New[string]()instead ofmap[string]bool.The repository guidelines forbid hand-rolled sets. Replace the map with
k8s.io/apimachinery/pkg/util/sets.♻️ Proposed refactor
- testNames := make(map[string]bool) - for _, test := range tests { - testNames[test.Name] = true - } - assert.True(t, testNames["[sig-api] pods should start"]) - assert.True(t, testNames["[sig-network] services should work"]) + testNames := sets.New[string]() + for _, test := range tests { + testNames.Insert(test.Name) + } + assert.True(t, testNames.Has("[sig-api] pods should start")) + assert.True(t, testNames.Has("[sig-network] services should work"))Add the import:
"k8s.io/apimachinery/pkg/util/sets"As per coding guidelines: "Use
k8s.io/apimachinery/pkg/util/setsfor deduplicating or collecting unique strings; do not usemap[string]boolas a hand-rolled set."🤖 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 `@test/integration/pgwriter_test.go` around lines 170 - 175, Replace the hand-rolled testNames map in the test with a k8s.io/apimachinery/pkg/util/sets Set created via sets.New[string](), add the required import, and update the membership assertions to use the set API while preserving the existing test-name checks.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@pkg/db/dailysummary/dailysummary_test.go`:
- Around line 48-63: Expand TestBackfill_UsesExplicitDateRange to assert every
expected date-and-release pair, ensuring each date in the inclusive range is
dispatched exactly once for both releases rather than relying only on count and
boundaries. Add a failure case using fakeStore so ReplaceRangeForRelease returns
an error, and assert backfillSummaries propagates that error.
---
Nitpick comments:
In `@test/integration/pgwriter_test.go`:
- Around line 170-175: Replace the hand-rolled testNames map in the test with a
k8s.io/apimachinery/pkg/util/sets Set created via sets.New[string](), add the
required import, and update the membership assertions to use the set API while
preserving the existing test-name checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 912ff36c-3844-4f1a-80c5-286652abefa9
📒 Files selected for processing (11)
pkg/dataloader/prowloader/accumulate_test.gopkg/dataloader/prowloader/pgwriter/pgwriter.gopkg/dataloader/prowloader/prow.gopkg/db/cumulativesummary/cumulative_summary.gopkg/db/cumulativesummary/cumulative_summary_test.gopkg/db/cumulativesummary/date_range.gopkg/db/cumulativesummary/date_range_test.gopkg/db/dailysummary/dailysummary.gopkg/db/dailysummary/dailysummary_test.gopkg/sippyserver/server.gotest/integration/pgwriter_test.go
💤 Files with no reviewable changes (3)
- pkg/db/cumulativesummary/cumulative_summary_test.go
- pkg/db/cumulativesummary/date_range_test.go
- pkg/db/cumulativesummary/date_range.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/sippyserver/server.go
- pkg/dataloader/prowloader/accumulate_test.go
- pkg/dataloader/prowloader/pgwriter/pgwriter.go
- pkg/dataloader/prowloader/prow.go
Extract test result insertion and summary table updates into a new pgwriter package. Daily totals and cumulative summaries are now refreshed incrementally within each prow load transaction, replacing the separate full-refresh step. The old Refresh() entry points and their auto-detection logic (date_range.go, resolveStartDate, etc.) are removed; only the explicit Backfill() path remains for historical repairs. Adds integration tests for lifecycle-scoped summaries, carry-forward across releases, future-dated batch rejection, and parallel release processing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
cefdb9a to
ac76eb7
Compare
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
|
@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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mstaeble, neisw, petr-muller 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 |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
4 participants
Summary
test_daily_totalsandtest_cumulative_summariesincrementally as part of each prow load batch, replacing the separate full-refresh step that re-scanned the entireprow_job_run_teststablepgwriterpackage with focused, single-responsibility functions for each SQL steperrgroup.Group) to reduce carry-forward time from ~3 minutes to ~50 secondscurrentDateat load start and threads it through all SQL to prevent midnight-crossing corruptionStaging validation
Tested against the staging database with multiple lookback windows:
Test plan
make lintpasses (0 issues)go test ./pkg/...passesmake integrationpasses (150 tests)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes