-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the activity queue generic over a trait #64
Conversation
17eb3f8
to
c3416ef
Compare
c3416ef
to
57d5b86
Compare
I dont see whats the benefit of allowing library users to implement their own activity queue. This seems completely orthogonal to the persistent storage unless Im missing something. |
@Nutomic at the moment the queue is extremely opinionated, with retries etc.. I've had to cut an image to turn off the retry queue a few times now to help keep load down, which signals to me that the opinions this library has is not a one-size-fits-all for everyone. Having the activity queue as a trait allows the queue itself to implement persistence & whatever logic possible. Once it receives a task it can do whatever it wants with it: whether that's ignore it because it's a bad host, prioritise it based on activity type, etc... In fact, there are already two implementations of the activity queue:
I think having a clear separation of concerns for the queue itself will mean that this is much more extendable. I also think that including "sensible defaults" is good as well, and this PR as it stands does not actually change things too much in that way. With some further work to this PR, including sorting out some traits/interfaces for the worker send tasks, I can show what the "persistence" implementation would look like. But it would not be implemented here, but in lemmy itself, since it relies on the postgres schema in lemmy. In other words: this PR's direction will allow lemmy to have a persistence queue, tailored for lemmy itself. |
The queue will still be opinionated if we move it into Lemmy instead. And disabling retries can be done with a simple setting. I only closed that PR because it didnt seem to give any performance benefit, but we can reopen it if desired. For implementing persistence it should be totally sufficient to have a trait like the one mentioned in the issue so that the application can take care of storage. Again, bring-your-own-queue seems completely orthogonal and unrelated to that so I dont really see the benefit. |
I'm not sure how to integrate I also don't know that the shape of the trait as mentioned will give enough freedom to implement enough control or allow things like having a worker in another process handle the queue. In other words: without making things messier, I don't know the best way to implement just the persistence layer in a generic way, since the machinations of how the current queue works apply a great deal of surface area to the persistence implementation. It's entirely understandable if you don't want to head in this direction as it is a bit of a change. I will close off this PR for now |
Note: this is a bit of an experiment and very much a WIP, but the main idea here is that you can BYO activity queue rather than use the embedded one.
The trait is rather simple (& I think we can get rid of the
stats
method as well):This does expose the existing queue type as
SimpleQueue
that people can use. But there are no helper methods for workers etc.. at this stage & would be part of this PR but need a bit of fleshing out.Something like this or similar would be needed for #31