Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

cetra3
Copy link
Contributor

@cetra3 cetra3 commented Jul 7, 2023

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):

#[async_trait]
/// Anything that can enqueue outgoing activitypub requests
pub trait ActivityQueue {
    /// The errors that can be returned when queuing
    type Error;

    /// Retrieve the queue stats
    fn stats(&self) -> &Stats;

    /// Queues one activity task to a specific inbox
    async fn queue(&self, message: SendActivityTask) -> Result<(), Self::Error>;
}

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

@cetra3 cetra3 marked this pull request as draft July 7, 2023 07:32
@cetra3 cetra3 force-pushed the generic_activity_queue branch from 17eb3f8 to c3416ef Compare July 8, 2023 01:44
@cetra3 cetra3 force-pushed the generic_activity_queue branch from c3416ef to 57d5b86 Compare July 8, 2023 01:49
@Nutomic
Copy link
Member

Nutomic commented Jul 10, 2023

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.

@cetra3
Copy link
Contributor Author

cetra3 commented Jul 10, 2023

@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:

  • the debug queue which sends things basically straight away and awaits the task until done
  • the current retry 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.

@Nutomic
Copy link
Member

Nutomic commented Jul 11, 2023

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.

@cetra3
Copy link
Contributor Author

cetra3 commented Jul 12, 2023

I'm not sure how to integrate PersistentFederationQueue into the existing code without making something generic. We could have the persistent queue as a member of ActivityQueue but then we would either need it to be a dyn or some other way of making sure we handle a generic implementation, which we'd need to feed into the FederationConfig.

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

@cetra3 cetra3 closed this Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants