LemmyNet / activitypub-federation-rust

High-level Rust library for the Activitypub protocol
GNU Affero General Public License v3.0
420 stars 45 forks source link

Make the activity queue generic over a trait #64

Closed cetra3 closed 1 year ago

cetra3 commented 1 year ago

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 https://github.com/LemmyNet/activitypub-federation-rust/issues/31

Nutomic commented 1 year ago

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 commented 1 year ago

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

Nutomic commented 1 year ago

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 commented 1 year ago

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