TRT-2741: Add partitioned prefix sum tables for matview refresh optimization#3762
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@mstaeble: This pull request references TRT-2741 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-robot
added
the
jira/valid-reference
WalkthroughAdds civil-date daily and cumulative summary aggregation, partitioned storage, refresh orchestration, and CLI/OpenShift backfill workflows. ChangesSummary pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant BackfillScript
participant BackfillCommand
participant PostgreSQL
Operator->>BackfillScript: provide table and batching options
BackfillScript->>BackfillCommand: launch sippy backfill with date range
BackfillCommand->>PostgreSQL: ensure partitions
BackfillCommand->>PostgreSQL: write selected summary table
Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 3 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 |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
mstaeble
force-pushed
the
trt-2741-prefix-sum-tables
branch
from
8d46f4f to
844e9a6
Compare
mstaeble
changed the title
mstaeble
force-pushed
the
trt-2741-prefix-sum-tables
branch
2 times, most recently
from
30b569d to
ab4daf2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
pkg/db/dailysummary/dailysummary.go (3)
200-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a brief godoc for
startDateFromMax.The function always caps the returned start at
yesterdayeven whenmaxSummaryis more recent (or invalid/in the future) — this "always re-process at least yesterday+today" behavior isn't obvious from the name alone.📝 Suggested doc comment
+// startDateFromMax returns the earliest date to re-aggregate. It always +// includes at least yesterday and today (since "today" data is still +// arriving), and falls back to defaultLookbackDays when the table is empty. func startDateFromMax(maxSummary *civil.Date, today civil.Date) civil.Date {🤖 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.go` around lines 200 - 209, Add a brief Go doc comment immediately above startDateFromMax describing that it derives the processing start date and caps maxSummary at yesterday, ensuring yesterday and today are reprocessed; mention the default lookback behavior when maxSummary is nil.Source: Path instructions
23-60: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueIdentifier interpolation via
fmt.Sprintf— safe here, but worth a defensive allow-list.
buildInsertSQL/buildOnConflictClause(andMaxSummaryDateat line 220) interpolatetableName/dateColumninto SQL text viafmt.Sprintf. Since these are compile-time literals from the twopgStore{...}construction sites in this file, there's no injection path today. As per path instructions,database/sqlcalls should use placeholders and avoidfmt.Sprintfin queries; since identifiers can't be parameterized, consider at least validatingtableName/dateColumnagainst a small allow-list inside these helpers as defense-in-depth against a future caller passing a non-literal value.🤖 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.go` around lines 23 - 60, Add defense-in-depth validation in buildInsertSQL and buildOnConflictClause, and apply the same validation to MaxSummaryDate, restricting tableName and dateColumn to the known allowed identifiers before interpolating them into SQL. Preserve the existing SQL generation for valid compile-time values and reject or fail safely for unsupported identifiers.Source: Path instructions
110-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
backfillSummariesanddoAggregateare near-duplicate day-loop implementations.Both query releases, log identical fields, loop day-by-day calling
aggregateReleases, and log elapsed time — differing only in log level (Info vs Debug) and the error-message prefix ("backfilling" vs "aggregating"). Consider extracting the shared day-loop into one helper parameterized by a log level/verb, reducing duplicated control flow.♻️ Suggested consolidation
func processDateRange(store summaryStore, releases []string, startDate, endDate civil.Date, skipConflictDetection bool, verb string, logFn func(args ...interface{})) error { for date := startDate; !date.After(endDate); date = date.AddDays(1) { dayStart := time.Now() if err := aggregateReleases(store, releases, date, date, skipConflictDetection); err != nil { return fmt.Errorf("%s %s: %w", verb, date, err) } log.WithFields(log.Fields{"date": date, "elapsed": time.Since(dayStart)}).Info("processed date") } return nil }🤖 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.go` around lines 110 - 167, Extract the duplicated date-processing loop from backfillSummaries and doAggregate into a shared helper, such as processDateRange, parameterized by releases, skipConflictDetection, and the operation verb. Preserve each caller’s error prefix and per-day log level (Info for backfill, Debug for aggregation), then have both functions invoke the helper while retaining their existing range-level logging.pkg/db/dailysummary/dailysummary_test.go (1)
187-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
sets.New[string]()instead ofmap[string]bool.
seen := make(map[string]bool)is a hand-rolled set for tracking processed releases. As per coding guidelines,k8s.io/apimachinery/pkg/util/sets(e.g.sets.New[string]()) should be used instead ofmap[string]boolfor this pattern.♻️ Suggested fix
- seen := make(map[string]bool) + seen := sets.New[string]() for _, call := range calls { - seen[call.release] = true + seen.Insert(call.release) assert.Equal(t, call.start, call.end, "each call should be a single day") } for _, rel := range releases { - assert.True(t, seen[rel], "release %s not processed", rel) + assert.True(t, seen.Has(rel), "release %s not processed", rel) }🤖 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 187 - 207, In TestRefresh_ProcessesAllReleasesPerDay, replace the hand-rolled seen map with a k8s.io/apimachinery/pkg/util/sets Set initialized via sets.New[string](). Add each call.release using the set API and update the release-presence assertion to use the set’s membership method.Source: Coding guidelines
pkg/db/db.go (1)
32-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTable-name constants duplicated as raw literals elsewhere.
partitionedTableTestDailyTotals/partitionedTableTestCumulativeSummariesare unexported here, sopkg/db/dailysummary/dailysummary.go("test_daily_totals") andpkg/db/cumulativesummary/cumulative_summary.go("test_daily_totals","test_cumulative_summaries") must re-declare the same strings as bare literals. A future rename here would silently desync from those packages (compiles fine, breaks at runtime).Consider exporting these as shared constants (e.g. in
pkg/dbor a small shared package) so all three files reference one source of truth.🤖 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/db.go` around lines 32 - 33, Export the partitioned table-name constants currently defined in db.go, then update dailysummary.go and cumulative_summary.go to reference those shared db constants instead of repeating raw table-name literals. Preserve the existing names and table mappings while establishing one source of truth for both daily and cumulative summary tables.pkg/db/cumulativesummary/cumulative_summary.go (1)
100-120: 🚀 Performance & Scalability | 🔵 TrivialEnsure
MAX(date)on the new partitioned tables uses an index-friendly plan.
MaxCumulativeSummaryDate/MaxDailySummaryDaterun an unfilteredMAX(date)over partitioned tables on every refresh. Worth confirming this reliably uses a per-partition index scan (e.g. viaenable_partitionwise_aggregateor a backward index scan on the newest partition) rather than scanning every partition, especially as partition count grows over time.🤖 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/cumulativesummary/cumulative_summary.go` around lines 100 - 120, Update MaxCumulativeSummaryDate and MaxDailySummaryDate so MAX(date) on the partitioned tables uses an index-friendly per-partition plan, such as enabling partitionwise aggregation or issuing a backward index scan against the newest partition. Verify the resulting queries avoid scanning every partition as partition count increases while preserving the existing return and error behavior.pkg/db/cumulativesummary/date_range_test.go (1)
1-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM! The tests cover all main branches of
resolveStartDatewith descriptive names and a fixedtestTodayfor determinism.Optionally, these could be consolidated into a table-driven test for conciseness and easier extension, per the project's preference for table-driven tests.
♻️ Optional table-driven refactor
func TestResolveStartDate(t *testing.T) { - earliestChanged := testToday.AddDays(-3) - result := resolveStartDate(earliestChanged, nil, testToday) - assert.Equal(t, earliestChanged, result) -} - -func TestResolveStartDate_UsesMaxPlusOneWhenEarlier(t *testing.T) { - earliestChanged := testToday.AddDays(-1) - maxExisting := testToday.AddDays(-5) - result := resolveStartDate(earliestChanged, &maxExisting, testToday) - assert.Equal(t, maxExisting.AddDays(1), result) -} - -func TestResolveStartDate_UsesEarliestChangedWhenEarlierThanMax(t *testing.T) { - earliestChanged := testToday.AddDays(-5) - maxExisting := testToday.AddDays(-2) - result := resolveStartDate(earliestChanged, &maxExisting, testToday) - assert.Equal(t, earliestChanged, result) -} - -func TestResolveStartDate_ClampsToFloor(t *testing.T) { - earliestChanged := testToday.AddDays(-30) - result := resolveStartDate(earliestChanged, nil, testToday) - assert.Equal(t, testToday.AddDays(-maxAutoFillDays), result) -} - -func TestResolveStartDate_ClampsMaxPlusOneToFloor(t *testing.T) { - earliestChanged := testToday.AddDays(-1) - maxExisting := testToday.AddDays(-30) - result := resolveStartDate(earliestChanged, &maxExisting, testToday) - assert.Equal(t, testToday.AddDays(-maxAutoFillDays), result) -} - -func TestResolveStartDate_DoesNotClampWhenWithinFloor(t *testing.T) { - earliestChanged := testToday.AddDays(-7) - result := resolveStartDate(earliestChanged, nil, testToday) - assert.Equal(t, earliestChanged, result) + tests := []struct { + name string + earliestChanged civil.Date + maxExisting *civil.Date + want civil.Date + }{ + {"UsesEarliestChanged", testToday.AddDays(-3), nil, testToday.AddDays(-3)}, + {"UsesMaxPlusOneWhenEarlier", testToday.AddDays(-1), ptrDate(testToday.AddDays(-5)), testToday.AddDays(-4)}, + {"UsesEarliestChangedWhenEarlierThanMax", testToday.AddDays(-5), ptrDate(testToday.AddDays(-2)), testToday.AddDays(-5)}, + {"ClampsToFloor", testToday.AddDays(-30), nil, testToday.AddDays(-maxAutoFillDays)}, + {"ClampsMaxPlusOneToFloor", testToday.AddDays(-1), ptrDate(testToday.AddDays(-30)), testToday.AddDays(-maxAutoFillDays)}, + {"DoesNotClampWhenWithinFloor", testToday.AddDays(-7), nil, testToday.AddDays(-7)}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, resolveStartDate(tt.earliestChanged, tt.maxExisting, testToday)) + }) + } } + +func ptrDate(d civil.Date) *civil.Date { return &d }🤖 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/cumulativesummary/date_range_test.go` around lines 1 - 61, Optionally consolidate the six resolveStartDate tests into a single table-driven test, preserving each existing scenario, expected result, descriptive case names, and the fixed testToday value. Keep the test focused on resolveStartDate without changing production behavior.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.
Inline comments:
In `@cmd/sippy/backfill.go`:
- Around line 67-70: Wrap the error returned by GetDBClient in the backfill flow
with call-site context using fmt.Errorf and the %w verb, while preserving the
existing early return behavior and error chain.
- Around line 43-90: Validate f.Table against the known supported table names
immediately after the existing required-field checks and before parsing dates or
obtaining the DB client. Use k8s.io/apimachinery/pkg/util/sets to define/check
the supported table set, return a clear error for unsupported values, and leave
valid-table backfill behavior unchanged.
In `@pkg/db/cumulativesummary/cumulative_summary.go`:
- Around line 29-34: Update Backfill to require a continuous cumulative boundary
before calling refreshDateRange: verify that startDate-1 exists for every
release, fail fast when any prior-day boundary is missing, and avoid mutating
cumulative data in that case. Preserve normal backfill behavior when all release
boundaries are present.
In `@pkg/sippyserver/server.go`:
- Around line 405-430: Update refreshMaterializedViews and its refreshMatview
path to return and propagate materialized-view refresh errors instead of only
logging them. In RefreshData, handle the returned error from
refreshMaterializedViews and return it with context, preserving the existing
successful refresh flow.
- Around line 432-451: Define shared constants for the supported table names and
replace the duplicated literals in BackfillData and the corresponding seed-data
dispatch code. Add unit tests covering RefreshData and BackfillData dispatch for
each supported table and asserting unknown table names return an error.
In `@scripts/backfill-summaries.sh`:
- Around line 86-89: Update the batch_end_offset lower-bound clamp in the
backfill calculation to use 1 instead of 0, preventing the final batch from
including today when DAYS is not divisible by BATCH. Preserve the existing
offset calculation and all other batching behavior.
- Around line 107-125: Update the oc run invocation in the backfill execution
loop to explicitly redirect standard input from /dev/null while preserving --rm
-i and the existing output filtering. Ensure interactive shell usage cannot
leave the command waiting for input and stall the loop.
---
Nitpick comments:
In `@pkg/db/cumulativesummary/cumulative_summary.go`:
- Around line 100-120: Update MaxCumulativeSummaryDate and MaxDailySummaryDate
so MAX(date) on the partitioned tables uses an index-friendly per-partition
plan, such as enabling partitionwise aggregation or issuing a backward index
scan against the newest partition. Verify the resulting queries avoid scanning
every partition as partition count increases while preserving the existing
return and error behavior.
In `@pkg/db/cumulativesummary/date_range_test.go`:
- Around line 1-61: Optionally consolidate the six resolveStartDate tests into a
single table-driven test, preserving each existing scenario, expected result,
descriptive case names, and the fixed testToday value. Keep the test focused on
resolveStartDate without changing production behavior.
In `@pkg/db/dailysummary/dailysummary_test.go`:
- Around line 187-207: In TestRefresh_ProcessesAllReleasesPerDay, replace the
hand-rolled seen map with a k8s.io/apimachinery/pkg/util/sets Set initialized
via sets.New[string](). Add each call.release using the set API and update the
release-presence assertion to use the set’s membership method.
In `@pkg/db/dailysummary/dailysummary.go`:
- Around line 200-209: Add a brief Go doc comment immediately above
startDateFromMax describing that it derives the processing start date and caps
maxSummary at yesterday, ensuring yesterday and today are reprocessed; mention
the default lookback behavior when maxSummary is nil.
- Around line 23-60: Add defense-in-depth validation in buildInsertSQL and
buildOnConflictClause, and apply the same validation to MaxSummaryDate,
restricting tableName and dateColumn to the known allowed identifiers before
interpolating them into SQL. Preserve the existing SQL generation for valid
compile-time values and reject or fail safely for unsupported identifiers.
- Around line 110-167: Extract the duplicated date-processing loop from
backfillSummaries and doAggregate into a shared helper, such as
processDateRange, parameterized by releases, skipConflictDetection, and the
operation verb. Preserve each caller’s error prefix and per-day log level (Info
for backfill, Debug for aggregation), then have both functions invoke the helper
while retaining their existing range-level logging.
In `@pkg/db/db.go`:
- Around line 32-33: Export the partitioned table-name constants currently
defined in db.go, then update dailysummary.go and cumulative_summary.go to
reference those shared db constants instead of repeating raw table-name
literals. Preserve the existing names and table mappings while establishing one
source of truth for both daily and cumulative summary tables.
🪄 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: ac6762e2-c6a7-4607-963e-0cfc2f15918b
📒 Files selected for processing (20)
cmd/sippy/backfill.gocmd/sippy/load.gocmd/sippy/main.gocmd/sippy/refresh.gocmd/sippy/seed_data.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/db/db.gopkg/db/migrations/000006_partition_cumulative_tables.down.sqlpkg/db/migrations/000006_partition_cumulative_tables.up.sqlpkg/db/migrations/MANIFESTpkg/db/models/prow.gopkg/flags/postgres_benchmarking_test.gopkg/sippyserver/server.goscripts/backfill-summaries.shscripts/updatesuites.go
mstaeble
force-pushed
the
trt-2741-prefix-sum-tables
branch
from
ab4daf2 to
cfc1cec
Compare
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
mstaeble
force-pushed
the
trt-2741-prefix-sum-tables
branch
from
cfc1cec to
d5e4e2f
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)
93-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrengthen the assertion in
TestRefresh_IncrementalCapsAtYesterday.The test name implies verifying that the refresh caps at yesterday, but the assertion
calls[0].start.Before(future)is trivially true for any reasonable date. Add an assertion that the end date is at most yesterday:require.NotEmpty(t, calls) - assert.True(t, calls[0].start.Before(future)) + assert.True(t, calls[0].start.Before(future)) + today := civil.DateOf(time.Now().UTC()) + assert.True(t, calls[len(calls)-1].end.Before(today), + "end date should be capped at yesterday, not extend to today or beyond")🤖 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 93 - 103, Strengthen TestRefresh_IncrementalCapsAtYesterday by asserting that the first refresh call’s end date does not exceed yesterday, using the existing date representation and current-date helper. Keep the existing non-empty and start-before-future assertions unchanged.
🧹 Nitpick comments (1)
pkg/db/models/prow.go (1)
187-220: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
civil.Dateconsistently for date-only columns.
TestDailyTotal.Dateusestime.TimewhileTestCumulativeSummary.Dateusescivil.Date. Both map to PostgreSQLdatecorrectly, butcivil.Dateis semantically more precise for date-only columns and matches the downstream code pattern (e.g.,cumulativesummary.gousescivil.Datethroughout). Usingcivil.Datein both models would improve consistency across the new partitioned summary tables.This is optional since
TestDailyTotalmirrors the existingTestDailySummarywhich also usestime.Time, and the downstream write paths use raw SQL rather than GORM model methods.🤖 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 187 - 220, Update TestDailyTotal.Date from time.Time to civil.Date while preserving its date column mapping and constraints, matching TestCumulativeSummary.Date and downstream date-only usage. Leave the other model fields and schema tags unchanged.
🤖 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 93-103: Strengthen TestRefresh_IncrementalCapsAtYesterday by
asserting that the first refresh call’s end date does not exceed yesterday,
using the existing date representation and current-date helper. Keep the
existing non-empty and start-before-future assertions unchanged.
---
Nitpick comments:
In `@pkg/db/models/prow.go`:
- Around line 187-220: Update TestDailyTotal.Date from time.Time to civil.Date
while preserving its date column mapping and constraints, matching
TestCumulativeSummary.Date and downstream date-only usage. Leave the other model
fields and schema tags unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f733f060-15cf-46cb-867c-d7eaa31e7e02
📒 Files selected for processing (20)
cmd/sippy/backfill.gocmd/sippy/load.gocmd/sippy/main.gocmd/sippy/refresh.gocmd/sippy/seed_data.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/db/db.gopkg/db/migrations/000006_partition_cumulative_tables.down.sqlpkg/db/migrations/000006_partition_cumulative_tables.up.sqlpkg/db/migrations/MANIFESTpkg/db/models/prow.gopkg/flags/postgres_benchmarking_test.gopkg/sippyserver/server.goscripts/backfill-summaries.shscripts/updatesuites.go
🚧 Files skipped from review as they are similar to previous changes (14)
- pkg/db/cumulativesummary/date_range_test.go
- cmd/sippy/backfill.go
- pkg/db/migrations/000006_partition_cumulative_tables.up.sql
- pkg/db/cumulativesummary/date_range.go
- pkg/db/migrations/MANIFEST
- pkg/flags/postgres_benchmarking_test.go
- pkg/db/cumulativesummary/cumulative_summary.go
- pkg/db/db.go
- cmd/sippy/refresh.go
- scripts/backfill-summaries.sh
- pkg/db/dailysummary/dailysummary.go
- scripts/updatesuites.go
- pkg/db/cumulativesummary/cumulative_summary_test.go
- pkg/sippyserver/server.go
mstaeble
force-pushed
the
trt-2741-prefix-sum-tables
branch
3 times, most recently
from
baac3a7 to
4039a46
Compare
|
@coderabbitai resolve |
openshift-ci
Bot
added
the
ready-for-human-review
✅ Action performedComments resolved and changes approved. |
|
/test agentic-images |
|
/test images |
|
Scheduling required tests: |
| ProwJobID uint `gorm:"column:prow_job_id;not null"` | ||
| SuiteID uint `gorm:"column:suite_id;not null;default:0"` | ||
| Release string `gorm:"column:release;not null"` | ||
| Date civil.Date `gorm:"column:date;type:date;not null"` |
There was a problem hiding this comment.
Just noting the addition of civil.Date and the existing usage of time.Time including TestDailySummary for dates. Do you see us standardizing on civil.Date over time? If not is it worth introducing here?
There was a problem hiding this comment.
Yes, I am standardizing on civil.Date as the type used wherever dates are used.
#3716 has the wider effort.
| seedStart := time.Now().Add(-190 * 24 * time.Hour) | ||
| sippyserver.RefreshData(dbc, nil, false, dailysummary.Options{StartOverride: &seedStart}) | ||
| seedToday := civil.DateOf(time.Now().UTC()) | ||
| seedStart := seedToday.AddDays(-91) |
There was a problem hiding this comment.
Any idea why it went back 190 days previously? I'm fine with 91 if all works ok.
There was a problem hiding this comment.
The 4.19 release data is from -180 to -150 days ago.
The 4.20 release data is from -120 to -90 days ago.
The -190 days covered those releases.
I think that I changed this date to -91 days in the past for consistency with what would be produced in the real database. But it does mean that we would not be able to do CR queries against 4.19 and 4.20 data in phase 3 when the CR queries use the summary tables.
I'v changed it back to use -190 days.
…ization Introduces two new LIST(release)->RANGE(date) partitioned tables that store pre-aggregated daily totals and running prefix sums. These tables enable sub-second queries for any arbitrary date range via prefix sum subtraction, eliminating the need for matview refreshes that currently take 7+ minutes. New tables: - test_daily_totals: partitioned replacement for test_daily_summaries - test_cumulative_summaries: running totals from test_daily_totals Variant grouping happens at query time via JOIN to prow_jobs, which keeps variant changes immediately reflected without any rebuild step. Phase 1 is additive only: the new tables are populated alongside the existing test_daily_summaries table. Nothing reads from them yet. Later phases will switch query paths to use these tables and remove the old matview dependency. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mstaeble
force-pushed
the
trt-2741-prefix-sum-tables
branch
from
4039a46 to
f1c9d75
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/db/dailysummary/dailysummary.go (1)
110-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
backfillSummariesanddoAggregateare near-duplicate implementations.Both query releases, compute
days, log start/end/day/release counts, loop day-by-day callingaggregateReleases, and log a per-date summary — differing only in the log messages/levels and whetherskipConflictDetectionis hardcoded tofalsevs. passed through. This duplication risks divergence (e.g., a future fix to one loop not applied to the other).♻️ Suggested consolidation
-func backfillSummaries(store summaryStore, startDate, endDate civil.Date) error { - loadStart := time.Now() - releases, err := store.Releases() - if err != nil { - return fmt.Errorf("querying releases: %w", err) - } - - days := endDate.DaysSince(startDate) + 1 - log.WithFields(log.Fields{ - "start": startDate, - "end": endDate, - "days": days, - "releases": len(releases), - }).Info("backfilling daily summaries") - - for date := startDate; !date.After(endDate); date = date.AddDays(1) { - dayStart := time.Now() - if err := aggregateReleases(store, releases, date, date, false); err != nil { - return fmt.Errorf("backfilling %s: %w", date, err) - } - log.WithFields(log.Fields{ - "date": date, - "elapsed": time.Since(dayStart), - }).Info("backfilled daily summaries for date") - } - - log.WithField("elapsed", time.Since(loadStart)).Info("daily summary backfill complete") - return nil -} - -func doAggregate(store summaryStore, startDate, endDate civil.Date, skipConflictDetection bool, loadStart time.Time) error { - releases, err := store.Releases() - if err != nil { - return fmt.Errorf("querying releases: %w", err) - } - - days := endDate.DaysSince(startDate) + 1 - log.WithFields(log.Fields{ - "start": startDate, - "end": endDate, - "days": days, - "releases": len(releases), - }).Info("aggregating daily summaries") - - for date := startDate; !date.After(endDate); date = date.AddDays(1) { - dayStart := time.Now() - if err := aggregateReleases(store, releases, date, date, skipConflictDetection); err != nil { - return fmt.Errorf("aggregating %s: %w", date, err) - } - log.WithFields(log.Fields{ - "date": date, - "elapsed": time.Since(dayStart), - }).Debug("aggregated daily summaries for date") - } - - log.WithField("elapsed", time.Since(loadStart)).Info("daily summary refresh complete") - return nil -} +func aggregateDateRange(store summaryStore, startDate, endDate civil.Date, skipConflictDetection bool, action string, perDateLevel log.Level) error { + loadStart := time.Now() + releases, err := store.Releases() + if err != nil { + return fmt.Errorf("querying releases: %w", err) + } + + days := endDate.DaysSince(startDate) + 1 + log.WithFields(log.Fields{ + "start": startDate, + "end": endDate, + "days": days, + "releases": len(releases), + }).Infof("%s daily summaries", action) + + for date := startDate; !date.After(endDate); date = date.AddDays(1) { + dayStart := time.Now() + if err := aggregateReleases(store, releases, date, date, skipConflictDetection); err != nil { + return fmt.Errorf("%s %s: %w", action, date, err) + } + log.WithFields(log.Fields{ + "date": date, + "elapsed": time.Since(dayStart), + }).Log(perDateLevel, action+"ed daily summaries for date") + } + + log.WithField("elapsed", time.Since(loadStart)).Infof("daily summary %s complete", action) + return nil +}🤖 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.go` around lines 110 - 167, Consolidate the duplicated release-loading, range-logging, and day-by-day aggregation logic in backfillSummaries and doAggregate into a shared helper. Parameterize the helper for skipConflictDetection and the required overall/per-date log messages or levels, then have both callers delegate to it while preserving their existing error context and elapsed-time behavior.
🤖 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/dailysummary/dailysummary.go`:
- Around line 200-209: Update startDateFromMax to clamp any maxSummary older
than today.AddDays(-defaultLookbackDays) to that 14-day lookback boundary, while
preserving the existing yesterday behavior for recent summaries and the
empty-table fallback. Add a regression test covering a very stale maxSummary and
verifying the bounded start date used by refresh flows.
---
Nitpick comments:
In `@pkg/db/dailysummary/dailysummary.go`:
- Around line 110-167: Consolidate the duplicated release-loading,
range-logging, and day-by-day aggregation logic in backfillSummaries and
doAggregate into a shared helper. Parameterize the helper for
skipConflictDetection and the required overall/per-date log messages or levels,
then have both callers delegate to it while preserving their existing error
context and elapsed-time behavior.
🪄 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: ea4b77b5-026c-4d35-9746-d801a3a09c00
📒 Files selected for processing (20)
cmd/sippy/backfill.gocmd/sippy/load.gocmd/sippy/main.gocmd/sippy/refresh.gocmd/sippy/seed_data.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/db/db.gopkg/db/migrations/000006_partition_cumulative_tables.down.sqlpkg/db/migrations/000006_partition_cumulative_tables.up.sqlpkg/db/migrations/MANIFESTpkg/db/models/prow.gopkg/flags/postgres_benchmarking_test.gopkg/sippyserver/server.goscripts/backfill-summaries.shscripts/updatesuites.go
🚧 Files skipped from review as they are similar to previous changes (15)
- cmd/sippy/main.go
- pkg/db/migrations/MANIFEST
- pkg/db/migrations/000006_partition_cumulative_tables.up.sql
- pkg/db/models/prow.go
- cmd/sippy/seed_data.go
- cmd/sippy/load.go
- cmd/sippy/refresh.go
- pkg/db/db.go
- scripts/backfill-summaries.sh
- pkg/sippyserver/server.go
- pkg/db/cumulativesummary/cumulative_summary_test.go
- cmd/sippy/backfill.go
- scripts/updatesuites.go
- pkg/flags/postgres_benchmarking_test.go
- pkg/db/cumulativesummary/cumulative_summary.go
| func startDateFromMax(maxSummary *civil.Date, today civil.Date) civil.Date { | ||
| yesterday := today.AddDays(-1) | ||
| if maxSummary != nil { | ||
| if maxSummary.Before(yesterday) { | ||
| return *maxSummary, nil | ||
| return *maxSummary | ||
| } | ||
| return yesterday, nil | ||
| return yesterday | ||
| } | ||
|
|
||
| return now.AddDate(0, 0, -defaultLookbackDays), nil | ||
| return today.AddDays(-defaultLookbackDays) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
ast-grep outline pkg/db/dailysummary/dailysummary.go --view expandedRepository: openshift/sippy
Length of output: 1784
🏁 Script executed:
ast-grep outline pkg/db/cumulativesummary/date_range.go --view expandedRepository: openshift/sippy
Length of output: 331
🏁 Script executed:
rg -n "startDateFromMax|resolveStartDate|RefreshTotals|maxAutoFillDays|defaultLookbackDays" pkg/db -g '*.go'Repository: openshift/sippy
Length of output: 2364
🏁 Script executed:
sed -n '190,215p' pkg/db/dailysummary/dailysummary.goRepository: openshift/sippy
Length of output: 644
🏁 Script executed:
sed -n '1,80p' pkg/db/cumulativesummary/date_range.goRepository: openshift/sippy
Length of output: 1110
🏁 Script executed:
sed -n '1,180p' pkg/db/dailysummary/dailysummary_test.goRepository: openshift/sippy
Length of output: 4758
Clamp stale daily summary refreshes to the 14-day lookback
startDateFromMax only applies defaultLookbackDays when the table is empty; a stale maxSummary still drives an unbounded catch-up in refreshSummaries and RefreshTotals. Clamp older dates to today.AddDays(-defaultLookbackDays) here, and add a regression test for a very stale maxSummary.
🤖 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.go` around lines 200 - 209, Update
startDateFromMax to clamp any maxSummary older than
today.AddDays(-defaultLookbackDays) to that 14-day lookback boundary, while
preserving the existing yesterday behavior for recent summaries and the
empty-table fallback. Add a regression test covering a very stale maxSummary and
verifying the bounded start date used by refresh flows.
Source: Learnings
|
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 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
3 participants
Summary
test_daily_totalsandtest_cumulative_summariestest_daily_totalsis a partitioned replacement fortest_daily_summaries, populated in paralleltest_cumulative_summariesstores running prefix sums fromtest_daily_totals, enabling sub-second queries for any arbitrary date range via prefix sum subtraction (cum(end) - cum(start-1))prow_jobs, so variant changes are immediately reflected without any rebuild steptest_daily_summaries. Nothing reads from them yet. Later phases will switch query paths and remove the old matview dependency.sippy backfillCLI command andbackfill-summaries.shscript for populating the tablesRefreshDatanow returns errors and short-circuits on failureStaging validation
Correctness
test_daily_totalsvalues verified identical totest_daily_summariesacross 200 sampled rows (two releases, two dates)Performance (warm queries, prod-scale staging data)
Refresh pipeline
Test plan
make lintpassesmake testpassesmake e2epasses (108 tests, 1 skipped)sippy migratecreates partitioned tables on stagingsippy backfillcreates partitions via gopar and populates data correctly🤖 Generated with Claude Code
Summary by CodeRabbit