Clear budgets stats cache after adding/removing votes #5456
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
391b536
to
9315907
Compare
javierm
force-pushed
the
budgets_stats_cache
branch
6 times, most recently
from
070425f
to
a71c414
Compare
javierm
force-pushed
the
budgets_stats_cache
branch
6 times, most recently
from
a8d559a
to
269edb1
Compare
javierm
force-pushed
the
budgets_stats_cache
branch
5 times, most recently
from
c277fb5
to
5d84035
Compare
javierm
force-pushed
the
budgets_stats_cache
branch
2 times, most recently
from
07f21e7
to
fb9516d
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
from
fb9516d
to
f7ff36f
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 hour, remove all the logic needed to manually expire them, and get rid of most of the issues related to the cache being outdated.
javierm
force-pushed
the
budgets_stats_cache
branch
from
f7ff36f
to
7524463
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. But, since now we're using a temporary table to calculate the stats, generating them might take between 1 and 2 seconds even in processes with a hundred thousand participants, meaning we can disable the cache and generate the stats on every request instead. It might not scale so well when there are millions of participants; we might re-enable the cache or find a different way to optimize the stats calculations if we ever get to that point. Note that, since we're only using the temporary table inside the `generate` method, we need to cache the results obtained inside this method so they're available elsewhere. We're using instance variables for this purpose. This means the code doesn't improve much with this change, but at least we don't have to worry about when we should expire the cache. Since loading stats in the admin section is fast even without the cache because they only load very basic statistics, we're not even generating them first 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
7524463
to
af8a78c
Compare
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
Issue #5393
Objectives
Invalidate the stats cache whenever a new vote is added or removed.
Notes for reviewers
There's an alternative to the last commit in the remove_budgets_admin_stats_cache branch.
Notes
Closes #5393