Add Cleaner task to clean completed invocations #1845
Conversation
There was a problem hiding this comment.
Thanks a lot for fixing the problem with cleaning up completed invocations @slinkydeveloper. The current solution halves the number of clean up messages needed for invocations that we retain. In the future we might be able to further reduce it by propagating only the time through Bifrost and then let every replica do the scanning themselves.
The changes look good to me. I had a few minor comments. +1 for merging after resolving them and making GHA pass.
Expanded the data structure StatusTimestamps that tracks changes within the InvocationStatus. Because those timestamps are currently not agreed between replicas, the functions are marked unsafe and should really be used only in cases where those timestamps don't influence the deterministic business logic of the PP. Also only the NeoInvocationStatus stores those fields, the old invocation status will never fill those. Now CompletedInvocation propagates the `CompletionRetention`. This field is filled with a `Duration::MAX` for the old invocation status, causing the addition to overflow (thus not causing the cleanup in the new cleaner task). "Time is what the clock says" A. Einstein
…r for NeoInvocationStatus.
slinkydeveloper
force-pushed
the
issues/1714
branch
from
f486c1d to
069ba7c
Compare
slinkydeveloper
force-pushed
the
issues/1714
branch
from
b60eee5 to
069ba7c
Compare
There was a problem hiding this comment.
Thanks for updating the PR @slinkydeveloper. The test looks good to me. I had a few minor questions. Please take a look.
slinkydeveloper
force-pushed
the
issues/1714
branch
from
c21f618 to
495332f
Compare
There was a problem hiding this comment.
Thanks for updating the PR @slinkydeveloper. I think the test could be a bit more robust and testing a non-zero retention duration but maybe this is a follow up.
| // By yielding once we let the cleaner task run, and perform the cleanup | ||
| tokio::task::yield_now().await; |
There was a problem hiding this comment.
What I don't like about this test is that it depends on the fact that Interval immediately triggers and that the task has been registered when we reach this point. Maybe it is what it is but if we had the clock used by the cleaner under our control, then we could make the test more robust.
There was a problem hiding this comment.
I think this is ok for the time being
Fix #1714, based on #1843