LB-323: Allow users to request statistics calculation #369
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
|
Renamed the scheduler container because it isn't used in production either. Makes sense, I think. |
mayhem
requested changes
Feb 28, 2018
There was a problem hiding this comment.
This is a very good start, but I think there are some things missing:
- Messages talk about "next interval", but provide no details on how long an interval is or when the next one would start. It might be nice to state when the next interval starts, or at the very least state what the schedule is.
- Once a user gets added to the queue for calculating stats, there is no way for them to know they've been added. They could also request to calculate stats again, which is no big deal, since the second request would be ignored. But this is a bad user experience. How could we give the user feedback that they are in the queue? Maybe tell them how many people are in the queue ahead of them?
Also, add tests.
Also, fix a few typoes
Also, remove the ability to request statistics if the stats in the db are very recent.
paramsingh
force-pushed
the
request-stats
branch
from
ef28214
to
43302cc
Compare
mayhem
requested changes
Mar 8, 2018
| queue=current_app.config['STATS_QUEUE'], | ||
| error_msg='Could not put user %s into statistics calculation queue, please try again later', | ||
| ) | ||
| _redis.redis.set(construct_stats_queue_key(current_user.musicbrainz_id), 'queued') |
There was a problem hiding this comment.
Now that we merged the brainzutils PR, should this be updated to use brainz util caching instead?
There was a problem hiding this comment.
This isn't exactly caching which is why I used our RedisConnection directly similar to how we use it for storing now_playing listens.
| flash.info('You have already been added to the stats calculation queue! Please check back later.') | ||
| elif db_stats.valid_stats_exist(current_user.id): | ||
| flash.info('Your stats were calculated in the most recent stats calculation interval,' | ||
| ' please wait until the next interval! We calculate new statistics every Monday at 00:00 UTC.') |
There was a problem hiding this comment.
This is good, but out of date the second we change the cronjob. I think we should improve this in the future, but roll with it for now.
| # if no bigquery support, sleep | ||
| if not self.app.config['WRITE_TO_BIGQUERY']: | ||
| while True: | ||
| time.sleep(10000) |
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.
Summary
Problem
Solution
Add a stats_calculator script and send users to that queue.