Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

activity view: populate jobs in batches #3464

Closed
wants to merge 1 commit into from

Conversation

kalikiana
Copy link
Member

This is to avoid hitting resource limits by making too many ajax requests at the same time.

With a sufficient amount of job_ events in the activity view, the browser may become unresponsive and also throw errors because the requests fail.

See: https://progress.opensuse.org/issues/58304

This is to avoid hitting resource limits by making too many ajax requests at the same time.

See: https://progress.opensuse.org/issues/58304
Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely better than before, also the limit to 5000 results. I'm only wondering whether it would be better to create a new route which returns all the data we want to show at once (although it is also kind of nice to see already the first jobs while the rest is still loading).

@codecov
Copy link

codecov bot commented Oct 13, 2020

Codecov Report

Merging #3464 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3464      +/-   ##
==========================================
- Coverage   95.56%   95.52%   -0.04%     
==========================================
  Files         366      366              
  Lines       31556    31556              
==========================================
- Hits        30155    30143      -12     
- Misses       1401     1413      +12     
Impacted Files Coverage Δ
t/lib/OpenQA/Test/Utils.pm 65.24% <0.00%> (-3.71%) ⬇️
t/lib/OpenQA/SeleniumTest.pm 87.43% <0.00%> (-0.55%) ⬇️
t/05-scheduler-full.t 97.43% <0.00%> (-0.52%) ⬇️
t/34-developer_mode-unit.t 99.48% <0.00%> (+0.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03de4fa...a5b89f4. Read the comment docs.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You managed to introduce four different magic numbers in this seemingly simple change. This looks it could be done better. Also, do we really need to manually call a for loop and do the calculation this way in javascript code? Isn't that such a common problem that a better approach must exist including the backing off with timeout?

@kalikiana
Copy link
Member Author

You managed to introduce four different magic numbers in this seemingly simple change. This looks it could be done better. Also, do we really need to manually call a for loop and do the calculation this way in javascript code? Isn't that such a common problem that a better approach must exist including the backing off with timeout?

I'm not aware that standard Javascript has a natural solution for that. I think we can hold off on this for now anyway since it only affects extreme cases and regular users seem to be okay with the current code. And we can revisit it later if needed.

@kalikiana kalikiana closed this Oct 16, 2020
@Martchus
Copy link
Contributor

I would at least add &length=5000 to the query. It is really not nice to have a page which doesn't scale or has no limit.

@kalikiana
Copy link
Member Author

kalikiana commented Oct 16, 2020

I would at least add &length=5000 to the query. It is really not nice to have a page which doesn't scale or has no limit.

That on its own will not address the issues. Even the search can handle more than that with more data. It's the too many individual requests being made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants