TRT-2821: Optimize GetJobRunTestsCountByLookback using cumulative summaries - #3876
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
Skipping CI for Draft Pull Request. |
|
@mstaeble: This pull request references TRT-2821 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. |
|
Warning Review limit reached
Next review available in: 23 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesLookback test count
Estimated code review effort: 4 (Complex) | ~30 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 19 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
openshift-ci
Bot
added
the
approved
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/api/tests.go`:
- Around line 384-460: Add regression coverage for the redesigned count
function, exercising multiple releases with overlapping test IDs and
date-boundary data. Verify the result globally deduplicates IDs and counts only
rows whose prefix-sum difference is positive, while preserving expected job-run
counts and successful concurrent aggregation.
- Around line 413-450: Update the concurrent release-processing flow around the
errgroup worker and testIDs aggregation so each worker inserts its
releaseTestIDs into the shared deduplicated testIDs set under a mutex before
returning. Initialize the set and mutex before launching workers, remove the
result channel and post-Wait aggregation, and preserve the existing
g.SetLimit(4) and error handling.
- Around line 392-395: Validate dbc for nil at the start of the surrounding test
function, before the prow_job_runs query dereferences dbc.DB, and return the
function’s existing error result with an appropriate error when it is nil.
Preserve the current query behavior for non-nil database clients.
🪄 Autofix
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: 5c9f680b-2ea7-4afd-811c-78c24a38b77b
📒 Files selected for processing (1)
pkg/api/tests.go
3ff9ca0 to
9383da7
Compare
mstaeble
changed the title
9383da7 to
e1f3394
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
e1f3394 to
ca2d801
Compare
openshift-ci
Bot
added
the
ready-for-human-review
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
ca2d801 to
2747b9b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/integration/lookback_count_test.go (1)
80-98: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd negative-delta coverage.
This test verifies that a zero delta is excluded. It does not verify that a negative delta is excluded. A change from
> 0to!= 0would pass this test and violate the positive-delta contract.Add a case where the end prefix sum is lower than the start prefix sum. Assert that the test ID count is zero.
As per coding guidelines, “New or modified functionality should include test coverage.”
🤖 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/lookback_count_test.go` around lines 80 - 98, Extend TestLookbackCount_ZeroDeltaExcluded with a fixture whose end-date prefix sum is lower than its start-date prefix sum, then assert GetJobRunTestsCountByLookback returns a zero test ID count for that negative-delta case. Preserve the existing zero-delta and positive-delta coverage, ensuring the test validates that only strictly positive deltas are counted.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 `@test/integration/lookback_count_test.go`:
- Around line 60-61: Remove the UTC-midnight race in the lookback count tests by
making fixture setup and GetJobRunTestsCountByLookback use a stable date pair.
Prefer injecting a fixed clock through the API test seam; otherwise seed
fixtures for both dates that can result from the separate UTC date calculations.
Apply the same change to all today/startMinusOne calculations in the affected
tests.
---
Nitpick comments:
In `@test/integration/lookback_count_test.go`:
- Around line 80-98: Extend TestLookbackCount_ZeroDeltaExcluded with a fixture
whose end-date prefix sum is lower than its start-date prefix sum, then assert
GetJobRunTestsCountByLookback returns a zero test ID count for that
negative-delta case. Preserve the existing zero-delta and positive-delta
coverage, ensuring the test validates that only strictly positive deltas are
counted.
🪄 Autofix
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: 51592e48-fbf7-47f9-8cab-2cf74f5a1cd7
📒 Files selected for processing (2)
pkg/api/tests.gotest/integration/lookback_count_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/api/tests.go
|
Scheduling required tests: |
2747b9b to
1baefd4
Compare
1baefd4 to
9f61222
Compare
Replace the single COUNT(DISTINCT) query against prow_job_run_tests (the largest table) with two targeted queries: 1. Job run count from prow_job_runs directly (one row per run, indexed on timestamp) instead of COUNT(DISTINCT prow_job_run_id) over millions of test rows. 2. Test ID count using per-release cumulative summary self-joins run concurrently via errgroup. Pinning each query to a single release enables partition pruning to a single date sub-partition per side of the join, avoiding the cross-partition scan that causes 4+ seconds of planning time alone. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
9f61222 to
eeccb8e
Compare
|
Scheduling required tests: |
|
/test e2e |
|
[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 |
|
/test e22 |
|
/test e2e |
|
@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. |
3 participants
Summary
COUNT(DISTINCT)query againstprow_job_run_tests(the largest table, ~70M rows in a 14-day window) with two targeted queries:prow_job_runsdirectly (one row per run, indexed on timestamp): ~50msBenchmark results (staging, 3 iterations)
Approach
The old query scanned
prow_job_run_testswith twoCOUNT(DISTINCT)aggregates (no release filter, hitting all partitions). The new approach:prow_job_runs(much smaller table, indexed timestamp)release_definitionstest_cumulative_summarieswith a self-join on two dates (window end vs window start minus one day), pre-aggregating by test_id withGROUP BY+SUMbefore joining, so only ~14K aggregated rows join instead of ~2M raw rowssets.New[int64]()Pinning each query to a single release is critical: without it, the Postgres planner spends 4+ seconds just evaluating ~3,600 sub-partitions (36 releases x ~100 date ranges). A single global query takes ~7s (4.4s planning + 2.7s execution), while per-release queries run concurrently in ~2s total.
Test plan
gofmt -wandgo vetpassTest_BenchmarkIndividual/TestCountsByLookback(3 iterations each)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Performance
Monitoring