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
Don't use the cache in admin budget stats #5456
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
microweb10
force-pushed
the
budgets_stats_cache
branch
2 times, most recently
from
March 30, 2024 11:02
391b536
to
9315907
Compare
javierm
force-pushed
the
budgets_stats_cache
branch
6 times, most recently
from
April 8, 2024 16:40
070425f
to
a71c414
Compare
javierm
force-pushed
the
budgets_stats_cache
branch
6 times, most recently
from
April 15, 2024 12:08
a8d559a
to
269edb1
Compare
javierm
force-pushed
the
budgets_stats_cache
branch
5 times, most recently
from
April 18, 2024 20:25
c277fb5
to
5d84035
Compare
javierm
force-pushed
the
budgets_stats_cache
branch
2 times, most recently
from
April 24, 2024 22:24
07f21e7
to
fb9516d
Compare
javierm
force-pushed
the
budgets_stats_cache
branch
from
May 11, 2024 14:29
fb9516d
to
f7ff36f
Compare
javierm
force-pushed
the
budgets_stats_cache
branch
2 times, most recently
from
May 11, 2024 14:57
7524463
to
af8a78c
Compare
Back in commit 383909e, we said: > Even if this class looks very simple now, we're trying a few things > related to these stats. Having a class for it makes future changes > easier and, if there weren't any future changes, at least it makes > current experiments easier. Since there haven't been any changes in the last 5 years and we've found cases where using the GeozoneStats class results in a slightly worse performance, we're removing this class. The code is now a bit easier to read, and is consistent with the way we calculate participants by age. This reverts commit 383909e.
This code isn't used since commit e3063cd.
Debugging shows that the bottleneck in the stats calculation is the number of times we're querying the users table using the same array of IDs in the `where` condition but each time combined with other conditions. So we're inserting the results of querying the users table with the array of IDs in a temporary table and using this temporary table for the other calculations. When querying this temporary table, there's no need to filter for IDs anymore. For budget stats, the `generate` method is now about 10-20 times faster for a budget with 20,000 participants. For budgets with only a few dozen participants, there's no significant difference in performance. I thought about modifying the `participants` method and use the temporary table there. The problem, however, is that in this case it isn't clear when to drop the temporary table, and we could end up with thousands of temporary tables in the database if we don't do it right. Creating and dropping the temporary table in the same transaction, on the other hand, guarantees that won't be the case. Note there's no risk of duplicate tables since they're created and dropped inside a transaction, so we're always using the same table name for the same resource. We're adding a test that fails with a `PG::DuplicateTable: ERROR: relation "participants__1"` error if we don't use a transaction.
Since we're doing many queries to get stats for each age group and each geozone, testing shows these indices make stats calculation about 25% faster on processes with 100,000 participants.
Cache is by default disabled on every non-production environment
This way we remove a bit of duplication and it'll be easier to change the `stats_cache` method.
Since now generating stats (assuming the results aren't in the cache) only takes a few seconds even when there are a hundred thousand participants, as opposed to the several minutes it took to generate them when we introduced the Cron job, we can simply generate the stats during the first request to the stats page. Note that, in order to avoid creating a temporary table when the stats are cached, we're making sure we only create this table when we need to. Otherwise, we could spend up to 1 second on every request to the stats page creating a table that isn't going to be used. Also note we're using an instance variable to check whether we're creating a table; I tried to use `table_exists?`, but it didn't work. I wonder whether `table_exists?` doesn't detect temporary tables.
javierm
force-pushed
the
budgets_stats_cache
branch
5 times, most recently
from
May 17, 2024 18:05
53fa57d
to
3551187
Compare
When we first started caching the stats, generating them was a process that took several minutes, so we never expired the cache. However, there have been cases where we run into issues where the stats shown on the screen were outdated. That's why we introduced a task to manually expire the cache. But now, generating the stats only takes a few seconds, so we can automatically expire them every day, remove all the logic needed to manually expire them, and get rid of most of the issues related to the cache being outdated. We're expiring them every day because it's the same day we were doing in public stats (which we removed in commit 631b48f), only we're using `expires_at:` to set the expiration time, in order to simplify the code. Note that, in the test, we're using `travel_to(time)` so the test passes even when it starts an instant before midnight. We aren't using `:with_frozen_time` because, in similar cases (although not in this case, but I'm not sure whether that's intentional), `travel_to` shows this error: > Calling `travel_to` with a block, when we have previously already made > a call to `travel_to`, can lead to confusing time stubbing.
javierm
force-pushed
the
budgets_stats_cache
branch
2 times, most recently
from
May 20, 2024 14:18
301ecb1
to
ba6c186
Compare
In commit e51e034, we started using the same code to show stats in the public area and in the admin area. However, in doing so we introduced a bug, since stats in the public area are only shown after a certain part of the process has finished, meaning the stats appearing on the page never change (in theory), so it's perfectly fine to cache them. However, in the admin area stats can be accessed while the process is still ongoing, so caching the stats will lead to the wrong results being displayed. We've thought about expiring the cache when new supports or ballot lines are added; however, that means the methods calculating the stats for the supporting phase would expire when supports are added/removed but the methods calculating the stats for the voting phase would expire when ballot lines are added/removed. It gets even more complex because the `headings` method calculates stats for both the supporting and the voting phases. So, since loading stats in the admin section is fast even without the cache because they only load very basic statistics, we're taking the simple approach of disabling the cache in this case, so everything works the same way it did before commit e51e034. Co-authored-by: Javi Martín <javim@elretirao.net>
javierm
force-pushed
the
budgets_stats_cache
branch
from
May 20, 2024 14:19
ba6c186
to
16f8445
Compare
javierm
changed the title
Clear budgets stats cache after adding/removing votes
Don't use the cache in admin budget stats
May 20, 2024
taitus
approved these changes
May 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Objectives