Add persistence to the help channel system#987
Merged
Conversation
added 2 commits
May 31, 2020 19:17
We're gonna need this for the help channel handling, and it seems like a reasonable type to support anyway. It requires a tiny bit of special handling, but nothing outrageous.
More specifically, we're turning three dicts into RedisCaches: - help_channel_claimants - unanswered - claim_times These will still work the same way, but will now persist their contents across restarts.
lemonsaurus
requested review from
jb3 and
kosayoda
and removed request for
a team
lemonsaurus
added
l: 1 - intermediate
p: 1 - high
MarkKoz
reviewed
Jun 6, 2020
MarkKoz
reviewed
Jun 6, 2020
ks129
suggested changes
Jun 6, 2020
This means we don't need to rely on strtobool, and is a cleaner implementation overall. Thanks @MarkKoz.
We're also switching from datetime.now() to datetime.utcnow().
Instead of first checking if the channel.id exists and then checking what it is, we just do a single API call, to prevent cases where something fucky might happen inbetween the first and the second call.
Since help_channel_claimants.delete will never raise a KeyError, it's not necessary to suppress one.
The datetime module returns a local timestamp for naïve datetimes. It has to be timezone-aware to ensure it will always be in UTC.
Future code will also need to get this time, so moving it out to a separate function reduces redundancy.
Moving this code into a separate function reduces redundancy down the line. This will also get used to re-scheduled cooldowns after a restart.
Using the cache is more efficient since it can check only the users it expects to have a cooldown rather than searching all guild members. Furthermore, re-scheduling the cooldowns ensures members experience the full duration of the cooldown. Previously, all cooldowns were removed, regardless of whether they were expired.
MarkKoz
reviewed
Jun 9, 2020
Contributor
|
I made a change to use the cache to remove expired cooldowns or re-schedule active cooldowns when the bot restarts. See b49f3e5. |
MarkKoz
approved these changes
Jun 9, 2020
Contributor
MarkKoz
left a comment
MarkKoz
left a comment
There was a problem hiding this comment.
Nice to see the cache being put to use. Excellent work. Only spotted one issue with the timestamps which I fixed myself.
ghost
added
the
kosayoda
approved these changes
Jun 16, 2020
Contributor
kosayoda
left a comment
kosayoda
left a comment
There was a problem hiding this comment.
LGTM. Implemented a trivial change suggested by Mark, and updated the docstring for RedisCache to reflect the new boolean type available for values.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
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.
This PR should ensure that help channel-related data is persisted across bot restarts.
To achieve this, we make two changes.
RedisCache now supports Booleans as values
For the unanswered cache, we need to know whether or not a channel has been answered, and so it was relevant to store a boolean in the RedisCache. There was no support for this, so I added some. It seems like a feature we might want in the future for other things anyway.
Refactoring help_channels.py
The rest is pretty straight forward - Replacing the
help_channel_claimants,unanswered, andclaim_timesdicts with RedisCaches, and changing the code that accessed it to do so in a RedisCache compatible way.