feat: limit job results on group overview and dashboard#7058
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7058 +/- ##
=======================================
Coverage 99.87% 99.87%
=======================================
Files 418 418
Lines 44000 44075 +75
=======================================
+ Hits 43945 44020 +75
Misses 55 55 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9cd5f55 to
1bbeeb7
Compare
|
Extended tests to cover the missing lines. EDIT: … or so I thought |
57dc1c2 to
d620f1c
Compare
b876a9e to
a509d1d
Compare
| if (defined $max_jobs_limit && $total_jobs_seen >= $max_jobs_limit) { | ||
| $limit_exceeded = $max_jobs_limit; | ||
| last; | ||
| } |
There was a problem hiding this comment.
| if (defined $max_jobs_limit && $total_jobs_seen >= $max_jobs_limit) { | |
| $limit_exceeded = $max_jobs_limit; | |
| last; | |
| } | |
| if (defined $max_jobs_limit && $total_jobs_seen >= $max_jobs_limit) { | |
| $limit_exceeded = $max_jobs_limit; | |
| last; | |
| } | |
| else { | |
| $remaining = $max_jobs_limit - $total_jobs_seen if defined $max_jobs_limit; | |
| } |
increases the visibility/readability and decreases reduntant checks. it is cleaner
There was a problem hiding this comment.
I don't understand. Removing the indendation will be invalid format. Maybe you just wanted to add the "else". But why does this additional "else" mixing pre-condition and post-condition increase visibility/readability?
There was a problem hiding this comment.
I need to say that many time I mark something as suggestion but it is not the exact code. it is more to show the intention. having said that, the purpose is to remove a second check as is in my $remaining = defined $max_jobs_limit ? $max_jobs_limit - $total_jobs_seen : undef; I think you can use only once the defined $max_jobs_limit and make it more easy to read and where the condition is involved.
There was a problem hiding this comment.
I need to say that many time I mark something as suggestion but it is not the exact code.
It makes it harder for everyone else to understand, though.
You could have just indented it to make it understandable. If you click on the suggestion icon, the code is already indented, so it's not even a lot of work.
Regarding the suggestion:
If you have
if ($xy) {
something;
last;
}
then an else branch is useless.
having said that, the purpose is to remove a second check as is in
my $remaining = defined $max_jobs_limit ? $max_jobs_limit - $total_jobs_seen : undef;
And it doesn't really reduce the number of times if defined $max_jobs_limit is called.
There was a problem hiding this comment.
you have a point (I think you are talking about semantics) but it doesnt address my initial review. the defined $max_jobs_limit appears twice. the question is whether can remove the second check (as Tina said even without else) and make it more clean/obvious/readable. it is small thing, which it will end up in the same functionality or leave it as is.
There was a problem hiding this comment.
as perlpunk also explained there is no benefit to the suggestion so leaving this as is
There was a problem hiding this comment.
but it doesnt address my initial review
I did.
the question is whether can remove the second check
Your suggestion does not remove a check.
Now this PR has been merged, so you can create another PR on top with your suggestion, then everyone can see what you mean.
a509d1d to
c21f771
Compare
8daa2c6 to
1dc79bd
Compare
Motivation: - Performance issues and 502 errors occurred when accessing job groups with a huge number of jobs (e.g. >10k). - Consistency with the tests overview page which already implements such limits. - Missing test coverage for boundary conditions and JSON response consistency in the initial implementation. Design Choices: - Implemented a limit on the number of jobs fetched for job group overview, parent group overview, and the dashboard. - Standardized limit reporting with a limit_exceeded flag in both HTML and JSON responses. - Optimized compute_build_results to respect remaining job limits across multiple groups and builds. - Refined limit-reached logic to correctly handle group boundaries on the dashboard. - Extended t/22-dashboard.t with comprehensive coverage for JSON reporting, boundary cases, and invalid regex error handling. Benefits: - Prevents web UI timeouts and improves server responsiveness for large datasets. - Clear user feedback via warning messages when results are truncated. - More robust and fully tested implementation of performance-critical limits. Related issue: https://progress.opensuse.org/issues/196913
1dc79bd to
96712f4
Compare
|
rebased, fixed conflicts, ready for review |
Implement a limit on the number of jobs fetched for job group overview,
parent group overview, and the dashboard (consistent with the tests
overview page). This prevents performance issues and 502 errors when
accessing job groups with a huge number of jobs (e.g. >10k).
A warning message is displayed to the user when results are truncated.
I had started in
until I realized that also the main index page aka "dashboard" of openQA
instances impacted. Also I found that we already have a limit for
/tests/overview that we can adapt here as well. And also regardless of
any optimization as done in #7026 we still benefit from a limit. I plan to
rebase #7026 on top and bump the default limit after optimization.
Related progress issue: https://progress.opensuse.org/issues/196913