Scheduler Redesign#1017
Conversation
Each instance now requires a name to be specified, which will be used as the suffix of the logger's name. This removes the need to manually prepend every log message with the name.
This is a major change which simplifies the interface. It removes the need to implement an abstract method, which means the class can now be instantiated rather than subclassed.
Makes them easier to identify when debugging.
The ability to use the `in` operator makes this obsolete. Callers can check themselves if a task exists before they try to cancel it.
It's redundant. After all, this scheduler cannot schedule anything else.
Naming it "task" is inaccurate because `create_task` accepts a coroutine rather than a Task. What it does is wrap the coroutine in a Task.
MarkKoz
added
t: feature
This prevents unawaited coroutine warnings.
The task is already popped from the dict, so there is no need to delete it afterwards.
The coroutine may cancel the scheduled task, which would also trigger the finally block. The coroutine isn't necessarily finished when it cancels the task, so it shouldn't be closed in this case.
|
Sentry issue: BOT-65 |
ghost
added
the
aeros
left a comment
aeros
left a comment
There was a problem hiding this comment.
I'll try to do a more in-depth look at it later, but here's a few comments for now:
Showing the task ID in the logs makes them distinguishable from logs for other tasks. The coroutine state is logged because it may come in handy while debugging; the coroutine inspection check hasn't been proven yet in production.
It'd fail to schedule the coroutine otherwise anyway. There is also the potential to close the coroutine, which may be unexpected to see for a coroutine that was already running (despite being documented).
aeros
left a comment
aeros
left a comment
There was a problem hiding this comment.
LGTM. I didn't get the chance to feature test this to ensure the scheduler still works 100% correctly after the changes (such as for help channels, mod tools, and reminders), but all of the main async logic looks correct. Nicely done, Mark.
The only additional thing I could think of would be some unit tests for the scheduler (I didn't see any in tests/bot/utils/, but I'm assuming some of the existing tests might use it). Without dedicated unit tests, a complete feature test would be particularly important to do prior to merging. I'll try to do so if I can set aside the time for it, but anyone else is welcome to in the meantime of course.
| delay = (time - datetime.utcnow()).total_seconds() | ||
| if delay > 0: | ||
| coroutine = self._await_later(delay, task_id, coroutine) |
There was a problem hiding this comment.
Not at all necessary and functionally equivalent, but this would be a good reason to use the walrus operator:
| delay = (time - datetime.utcnow()).total_seconds() | |
| if delay > 0: | |
| coroutine = self._await_later(delay, task_id, coroutine) | |
| if (delay := (time - datetime.utcnow()).total_seconds()) > 0: | |
| coroutine = self._await_later(delay, task_id, coroutine) |
I find that it's a minor improvement to readability by clearly communicating to the reader that the variable is only used within the scope of a statement, and makes the code a bit less repetitive.
There was a problem hiding this comment.
I disagree because it makes the expression far too complicated. I wouldn't mind it if the expression started out simpler, but it already had a set of parenthesis and two function calls which just turns into a blob of parenthesis at a glance.
There was a problem hiding this comment.
Yeah, I can see that it could exceed the threshold of "reasonable amount of parenthesis". Either way, it's fairly subjective and not a huge deal, which is why I approved of the PR without it.
There was a problem hiding this comment.
I prefer it the way it is - I don't think the walrus adds any readability here, rather the opposite.
|
Thanks for the review. I did test things again and everything still seems to be in order. I'm not keen on writing tests any more; I need to study more before I can feel confident in writing tests again. |
lemonsaurus
left a comment
lemonsaurus
left a comment
There was a problem hiding this comment.
Looks excellent to my eyes. Well documented, exceptional commit history, lots of logging and a clean solution.
Probably ready to merge.
| delay = (time - datetime.utcnow()).total_seconds() | ||
| if delay > 0: | ||
| coroutine = self._await_later(delay, task_id, coroutine) |
There was a problem hiding this comment.
I prefer it the way it is - I don't think the walrus adds any readability here, rather the opposite.
| self._log.debug(f"Explicitly closing the coroutine for #{task_id}.") | ||
| coroutine.close() | ||
| else: | ||
| self._log.debug(f"Finally block reached for #{task_id}; {state=}") |
There was a problem hiding this comment.
nice use of the new = format specifier.
4 participants
Resolves #800
The
Schedulermust be instantiated rather than inherited. The new API relies on a coroutine being passed to one of three functions:schedule()- schedule a coroutine immediately as aTaskschedule_at()- schedule a coroutine to run at a givendatetime.datetimeschedule_later()- schedule a coroutine to run after sleeping for a given number of secondsThe need to implement an abstract method has been done away with. The ability to pass an arbitrary coroutine makes the
Schedulerinherently more flexible. I wanted some discussion (#800 (comment)) on the signatures of these functions but I am content with the design I chose for the time being.__contains__was implemented to support checking for scheduled tasks by ID using theinoperator. With this new possibility, theignore_missingparameter ofcancelwas made redundant and removed.The
Schedulercarries over the coroutine closing logic fromHelpChannels, which was done to prevent unawaited coroutine warnings. A bug with it has also been fixed. Now all users of theSchedulercan take advantage of this.I'm aware an exception is raised when a reminder is deleted. I believe this exception was always being raised; it just wasn't visible until now. It will be fixed by #835.