PART 2: perf: handle huge job groups gracefully - After #7058#7026
PART 2: perf: handle huge job groups gracefully - After #7058#7026okurz wants to merge 2 commits intoos-autoinst:masterfrom
Conversation
b35dfa2 to
afa425c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7026 +/- ##
==========================================
- Coverage 99.87% 99.86% -0.01%
==========================================
Files 418 420 +2
Lines 44000 44190 +190
==========================================
+ Hits 43945 44132 +187
- Misses 55 58 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
49f42c8 to
3345729
Compare
Martchus
left a comment
There was a problem hiding this comment.
Look generally good. Have you tested this with production data and compared before and after? I think for bigger changes like that we should do that (like when I recently did a similar optimization for the test results overview page). This way we could also confirm whether it helps with the problem from the motivating ticket (https://progress.opensuse.org/issues/196709).
| $job_result->{total} = 0; | ||
| } | ||
|
|
||
| sub _get_job_result_category ($state, $result) { |
There was a problem hiding this comment.
I'm wondering whether we can reuse some of the mapping functions we already have to avoid repetitiveness here. Of course this is already better than how it was before your change.
3345729 to
38ddab6
Compare
Martchus
left a comment
There was a problem hiding this comment.
I have just some style nitpicks but enough to say that at least some of them should be improved.
| my @jobs_data; | ||
| push @jobs_data, {%common, id => 600000 + $_, TEST => "test_$_"} for (1 .. $num_jobs); | ||
| $schema->resultset('Jobs')->populate(\@jobs_data); |
There was a problem hiding this comment.
Use map or avoid the intermediate array by calling create in a loop.
| $t->get_ok("/group_overview/$group_id_ctrl" => form => {distri => 'distri', version => $version, build => $build}) | ||
| ->status_is(400)->content_like(qr/exceeds the limit of 5/); | ||
| }; | ||
| subtest 'API Controller limit enforcement' => sub { |
There was a problem hiding this comment.
This subtest is almost like the previous ones and they are all very verbose. It would make sense to avoid this kind of duplication.
| use Test::Mojo; | ||
| my $test_case = OpenQA::Test::Case->new; | ||
| my $schema = $test_case->init_data(fixtures_glob => '01-jobs.pl 03-users.pl'); | ||
| my $t = Test::Mojo->new('OpenQA::WebAPI'); | ||
| my $group_id = 1001; | ||
| my $group = $schema->resultset('JobGroups')->find($group_id); | ||
| subtest 'Aggregation with deduplication' => sub { |
There was a problem hiding this comment.
Please use blank lines to separate different sections.
| if ($state eq OpenQA::Jobs::Constants::DONE) { | ||
| my $meta = OpenQA::Jobs::Constants::meta_result($result); | ||
| return 'passed' if $meta eq OpenQA::Jobs::Constants::PASSED; | ||
| return 'softfailed' if $meta eq OpenQA::Jobs::Constants::SOFTFAILED; | ||
| return 'skipped' if $meta eq OpenQA::Jobs::Constants::ABORTED; | ||
| return 'failed' if $meta eq OpenQA::Jobs::Constants::FAILED || $meta eq OpenQA::Jobs::Constants::NOT_COMPLETE; | ||
| } | ||
| return 'skipped' if $state eq OpenQA::Jobs::Constants::CANCELLED; |
There was a problem hiding this comment.
It would look less noisy if the constants were imported.
d3flex
left a comment
There was a problem hiding this comment.
it seems to me that this will have impact in the performance. there are multiple $jobs_resultset->search which can stress the system with long results. is there anything better to avoid this? or do i miss something?
| if ($jr{children}) { | ||
| for my $child_id (keys %{$jr{children}}) { |
There was a problem hiding this comment.
| if ($jr{children}) { | |
| for my $child_id (keys %{$jr{children}}) { | |
| if (my $children = $jr{children}) { | |
| for my $child_id (keys %$children) { |
|
This pull request is now in conflicts. Could you fix it? 🙏 |
61ecbbb to
1cee06d
Compare
|
This pull request is now in conflicts. Could you fix it? 🙏 |
1cee06d to
24914b1
Compare
8754ad6 to
116ad1f
Compare
- Optimize compute_build_results by using database-level aggregation instead of fetching and iterating all job objects. - Move job deduplication (per scenario) to the database level. - Implement a safety limit of 5,000 jobs per build to prevent timeouts. - Optimize comment/review tracking by only checking failed jobs. - Add t/61-job_group_aggregation.t to verify aggregation and limit enforcement. - Extract category mapping into _get_job_result_category helper. - Reuse helper in both count_job and _count_job_aggregated. - Ensure consistent result categorization across legacy and optimized paths. - Add job_group_overview_max_jobs to misc_limits in openqa.ini. - Pass this limit from web and API controllers to compute_build_results. - De-duplicate common job data in t/61-job_group_aggregation.t. - Add controller and API tests for limit enforcement. Related progress issue: https://progress.opensuse.org/issues/196913
- Introduce OpenQA::Error::LimitExceeded typed exception for robust error handling. - Decompose compute_build_results into smaller, testable helper functions. - Consolidate database queries to reduce O(N) round-trips. - Centralize job limit configuration using app config and internal defaults. - Implement graceful degradation for oversized builds instead of hard failure.
116ad1f to
de7b0c6
Compare
instead of fetching and iterating all job objects.
Related progress issue: https://progress.opensuse.org/issues/196913
After: