JasperFx / wolverine

Supercharged .NET server side development!
https://wolverinefx.net
MIT License
1.26k stars 139 forks source link

IMessagePublisher / ICommandBus feedback / questions #104

Closed ejsmith closed 1 year ago

ejsmith commented 1 year ago

I've been digging into the code and had some feedback/questions.

  1. When I look at the main entry point to using Wolverine (IMessagePublisher and ICommandBus), IMO they are confusing when the rest of the system (configuration and handlers) are very simple and elegant. It feels like they could be simplified. Looking at the different messaging frameworks (NServiceBus, MassTransit) they feel overly complicated and Wolverine is doing an awesome job of simplifying and modernizing some things, I'd love to see the sending side of Wolverine be simpler and more intuitive as well.
  2. I don't understand the need for a separate ICommandBus set of methods for sending/invoking messages vs just using IMessagePublisher. Wouldn't you be able to check to see if there is a local handler for the message and if so invoke the message handler pipeline inline? I'm probably missing a reason as to why this is necessary.
  3. On IMessagePublisher it feels like a lot of the methods could be removed or maybe tucked under an advanced property to keep the primary use cases more intuitive. Things like SendToTopic, couldn't that just be a property on the DeliveryOptions?
  4. From what I see, there are 4 primary use cases that feel like they should be called out into separate methods and the rest feel like they should be part of the DeliveryOptions.
    1. Send a message and throw if no receivers.
    2. Publish a message without caring if anyone is listening.
    3. Send a message and request a response (RPC).
    4. Schedule a message to be published later.

Thoughts?

jeremydmiller commented 1 year ago

Let's see:

  1. ICommandBus is a subset of IMessagePublisher. The only point or intention of that was that you could have a tighter entry point if you're only using Wolverine in process and I thought that would be potentially helpful. I don't have a huge attachment to keeping them separate if it's confusing. Or if you're only using Wolverine as a "mediator", but I didn't want to call it "IMediator" like everything else does
  2. "Wouldn't you be able to check to see if there is a local handler for the message and if so invoke the message handler pipeline inline?" -- it already does that under the covers, but there's a little bit of potential user confusion if there's a mix of local and external messaging that hit early Jasper users. I do like keeping the explicit Enqueue() business as an alternative though so that it's very clear that this message is staying right here.
  3. Yes and no. I'm standing by the SendToTopic/Endpoint here. Partially as a little performance optimization in the routing, but also to make that operation be more readily discoverable. I don't think the DeliveryOptions thing is very discoverable at all is my concern.
  4. I think you probably identified the 4 most common usages, but there's the optimization issue and the SendToEndpoint/Topic stuff is somewhat in there to support quasi-agent programming that I happen to know is coming very soon with Marten + Wolverine.

As for possible changes,

I'm not feeling like any of this is a black and white "you should definitely do this". This feels more like a matter of taste to me. I don't think I would opt for any changes until there's more feedback one way or another. It's churn right now when the biggest need is more documentation IMO.

ejsmith commented 1 year ago

Thanks! I agree that this isn't black and white and that it's definitely subjective. I just feel like there is a huge opportunity to fill a gap in the existing options by providing a simpler, more intuitive messaging framework which you have already done in several areas (configuration, handlers, sagas). I feel like I have a decent amount of experiencing using messaging and it was confusing to me.

It seems like the primary issue is that there are more concepts than there need to be.

image

  1. IMessagePublisher vs ICommandBus I feel like this is unnecessary confusion. Especially that IMessagePublisher inherits ICommandBus and I see both sets of methods when using IMessagePublisher. I personally just don't see why these 2 things should be separate concepts and it seems like it would be easier to simply note that local handlers will be invoked inline. Also seems like this could be a great story for how you can easily scale your application from in process message handling to services through simple configuration changes.
  2. IMessagePublisher.RequestAsync vs ICommandBus.InvokeAsync From my understanding, the difference being that ICommandBus only works with local inline message handlers and RequestAsync works with out of process message handlers. It feels like it would be more intuitive to me to consolidate this down to InvokeAsync and in the docs state that if the handler is local in process that the entire thing will happen inline.
  3. IMessagePublisher.SchedulePublishAsync vs ICommandBus.ScheduleAsync It seems like these are the same thing besides one expecting local handlers. Feels like this could be consolidated down to just ScheduleAsync.
  4. EnqueueAsync Enqueue is a very overloaded term. When I think of enqueue, I'm just thinking this is a worker queue where there are subscribers picking these messages up and working on them. It doesn't imply to me at all that "this message is staying right here". IMO, I would remove this to keep things simpler, but obviously very subjective.
  5. IMessagePublisher.SendTo* I guess what you are saying is that having these will help with performance by reducing allocations vs having to create a DeliveryOptions instance with this. If so, for consistency shouldn't there be matching InvokeTo* and ScheduleTo* methods then? It feels like these use cases are very specialized and not something that many people would use and that they could be tucked away some where if they truly make a worthwhile difference for performance. I agree discoverability is important, but I guess the question is, how common do you think it would be for people to control things this way vs using conventions or message type based configuration? Are you maybe biasing this on your internal more advanced use cases vs what a normal user would do?
niemyjski commented 1 year ago

I also agree on 1., always was confusing to me why to have both and when I would use one or the other. I'd prefer to just have one.

jeremydmiller commented 1 year ago

Going off the numbering from @ejsmith's previous comment:

  1. IMessagePublisher vs ICommandBus -- I'm good with the merger, or here's an alternative. Rename ICommandBus to ILocalBus maybe. Definitely keep InvokeAsync() to mean "execute this right this second)". See the next for the InvokeAsync<T>() flavor

  2. For IMessagePublisher.RequestAsync vs ICommandBus.InvokeAsync (with the expected return value), how about InvokeAsync() becomes QueryAsync<T>(object query, CancellationToken token = default) : Task<T> to make it clearer what's going on? That's there specifically to reach feature parity w/ MediatR to be honest -- but I'm spotting some reason to want that for real systems at work right now too

  3. What about an IMessagePublisher.Local "hub" property that collects the specific actions that happen locally? The PublishAsync() is already "smart" enough to publish locally if there is no known, remote subscriber, but I'm adamant there needs to be a semantically separate way to say "put this in a local queue" w/o any confusion about what goes local vs. remote. The magic routing was a problem for an early Jasper adopter. So IMessagePublisher.Local.ScheduleExecutionAsync() maybe.

  4. Maybe put it underneath IMessagePublisher.Local.EnqueueAsync()?

  5. InvokeTo doesn't apply, that's automatically a local operation to do it inline at this point. I buy into Eric's comment here, but I'd propose using the Advanced trick we use in Marten and stole from Raven to put less often used API options kind of out of the way. So IMessagePublisher.Advanced.SendToTopic() -- except I think that one could easily be commonly used

And then lastly, if we combine the services -- and there's also IMessageContext that adds a couple operations that are only valid in the context of handling the current incoming message -- I think I'd vote for a name change. Maybe the inevitable IMessageBus ?

jeremydmiller commented 1 year ago

And I know I said hold off on this because docs are the most important thing for Wolverine just at this second, but there's a DotNetRocks episode on Wolverine dropping next week, so it wouldn't hurt to get any proposed API changes in sooner rather than later. Updating the docs after the code is updated is trivial at least.

ejsmith commented 1 year ago

So I think you are saying you don't want to combine methods that do the same thing remotely or inline because people want to explicitly call out that they are doing things inline and they would never want to simply make a config change and have a message switch from being handled inline to being handled in some out of process service?

I like the IMessageBus name. So you would consolidate the 2 interfaces down to that one and then tuck a few of the methods off into an Advanced section and also have a Local section where all of the local inline command bus methods go? Would you try and consolidate terminology and merged names between that and the distributed methods?

I really don't like the QueryAsync thing. To me, that is naming things to be applicable for the CQRS patterns instead of keeping this just about sending and receiving messages in various ways.

jeremydmiller commented 1 year ago

"So I think you are saying you don't want to combine methods that do the same thing remotely or inline because people want to explicitly call out that they are doing things inline and they would never want to simply make a config change and have a message switch from being handled inline to being handled in some out of process service?"

You're conflating a couple different operations maybe. You have the option to:

  1. Invoke the processing of a message right there and now, completely in the current thread. That's InvokeAsync() today. That needs to stay no matter what for testing and/or "mediator" scenarios
  2. Explicitly enqueue a message to local, in memory queues with or without bothering with any possible kind of routing configuration to express that. That's different than executing inline through the InvokeAsync() call.
  3. Publish a message which might or might not mean being sent to an in memory queue or a remote subscriber depending on a bit of configuration

I'm concerned about the magic maybe it's local, maybe it's remote because it confused early Jasper users already. That's why there is explicit methods for doing things locally vs the Send/Publish that just sends things out to whatever subscribers are known -- which could be local, could be remote.

One thing I'll definitely do is add an Advanced.PreviewRouting(message) that returns back the outbound destinations for the message as a diagnostic tool.

"I really don't like the QueryAsync thing. To me, that is naming things to be applicable for the CQRS patterns instead of keeping this just about sending and receiving messages in various ways." -- I don't see any big problem with that myself, and tbh, I'm explicitly planning on the Wolverine + Marten combo being a CQRS framework anyway. It's not really a request/reply thing here, it's executing a query executor locally. Could say ExecuteQueryAsync() I suppose if that's clearer. And again, that's for mediator type functionality.

jeremydmiller commented 1 year ago

Thinking maybe about the big goals where Wolverine would be useful:

  1. As a message bus, and I think that's where all your focus is here
  2. As a mediator within a single process, so the InvokeAsync() & InvokeAsync() that are executing things inline and in the second case returning a value. I haven't put much energy here yet, partially because I've always blown off MediatR as unnecessary complexity. But you want that functionality for testing even if you're only using Wolverine strictly as a messaging bus
  3. Doing quite a bit to enable asynchronous processing or data streaming within a single process (today's CommandBus stuff, the EnqueueAsync() method)
jeremydmiller commented 1 year ago

Proposal 1

I finally had some to think on this late in the day driving to pick up the kids after school.

Let's combine ICommandBus and IMessagePublisher into a single IMessageBus interface. I'll leave the old ones there and use an [Obsolete] marker on them temporarily.

Immediately under the new IMessageBus, let's leave:

Now, on the IMessageBus.Advanced:

In regards to sending, send & await-ing, or request/reply to a specific uri/endpoint, we could use a fluent interface to first pick the endpoint, then the operation. I'm kind of down on fluent interfaces at runtime (okay w/ config time), but this would do a lot to reduce the number of overloads.

ejsmith commented 1 year ago

That sounds like it would be a pretty solid simplification.

I'm going to throw my thoughts out as well. You are so much further into this than I am that I probably don't know what I'm talking about, but this is what I'm thinking.

  1. Message bus
    • Publish a message (pub/sub) PublishAsync
    • Send a message and throw if no receivers SendAsync
    • Send a message and wait for a response of a specific type InvokeAsync<T>
    • Schedule a future message ScheduleAsync
  2. Mediator / Command Bus (in process handlers)
    • Invoke a local message handler throw if no handler defined InvokeAsync
    • Invoke a local message handler and expect a response of a specific type InvokeAsync<T>
    • Publish a message (pub/sub) PublishAsync
    • Schedule a future message ScheduleAsync
    • Send a message without waiting (currently EnqueueAsync), option of being durable SendAsync
      • The queue part of this feels like an implementation detail. Wouldn't all handlers use a queue structure behind the scenes since you can receive messages in batches and you can also control parallelization of processing and back pressure for those handlers as well?
      • I know that currently you can only configure parallelization for subscriber implementations, but what if there was something like opts.Handlers.ConfigureHandlerForMessage<SomeMessage>().MaximumParallelMessages(10).UseDurableInbox()? So you wouldn't really think in terms of local queues, just in terms of how messages are handled.

So you have these concepts:

jeremydmiller commented 1 year ago

"I know that currently you can only configure parallelization for subscriber implementations, but what if there was something like opts.Handlers.ConfigureHandlerForMessage().MaximumParallelMessages(10).UseDurableInbox()? So you wouldn't really think in terms of local queues, just in terms of how messages are handled."

Kinda like that. It's just syntactical sugar. You could already do that today, but you'd start from the queue.

niemyjski commented 1 year ago

I really like these concepts as well. Apart of me thinks that Schedule should be an advanced option maybe on send? There are few transports that efficiently allow for scheduled sending (so it feels weird to be a first class concept). And if you are scheduling locally or trying to schedule with something that doesn't support delayed, what is the durability guarantees (outbox in this case, with the schedule being best attempt)?

So you have these concepts:

  • Publish = Fire and forget (pub/sub)
  • Send = I expect someone to do something with this message throw if no receiver / handler, local handlers are preferred over remote receivers. Doesn't wait for message to be handled. Can optionally wait for acknowledgment.
  • Invoke = Process this message immediately and wait for the handler to be called whether it's local or remote

    • For local handlers, this means the handler will get called inline using the current thread
    • For remote handlers, this is mainly used for RPC style where you will await a response and it feels as if the handler is called inline. Could wait for some sort of default completion message when called without a response type. So it wouldn't return until the remote handler has been run.
  • Schedule = Publish this message later. Doesn't seem like you'd want to throw if there wasn't a handler for this, but I guess you could check for a handler up front. So maybe there would be ScheduleSendAsync and SchedulePublishAsync.
ejsmith commented 1 year ago

Yeah, the more I've thought about this the more I really like these terms / concepts:

I think they are easy to understand and work consistently whether they are local or remote.

jeremydmiller commented 1 year ago

I think I'd vote for a separate, advanced InvokeRemotelyAsync() (Today's SendAndWait()), or leave that alone as is. Again, you want minimal surprised when the API is called.

"Use handler configuration to control durability and parallelism" -- that's only applicable to locally handled messages though, and to be technical, it is a separate queue. For the sake of some local usage where ordering becomes important, you would want/need to assign message types to local queue names. There's already an early wolverine user that utilizes that.

What I'd say is to make it easy (I'd argue it already is) to either configure local message handling by message type or let users have more control by assigning message types per queue.

Also very likely you'd want to specify exact queue names when publishing locally if there's higher or lower message priority.

I think I'd like to consider breaking Advanced into Local and Remote to make it even more clear what's happening. That eliminates the user surprise, and would let us use InvokeAsync() semantics for remote invocation -- not that I'd recommend folks use that too awfully much.

jeremydmiller commented 1 year ago

I really like these concepts as well. Apart of me thinks that Schedule should be an advanced option maybe on send? There are few transports that efficiently allow for scheduled sending (so it feels weird to be a first class concept). And if you are scheduling locally or trying to schedule with something that doesn't support delayed, what is the durability guarantees (outbox in this case, with the schedule being best attempt)?

So you have these concepts:

  • Publish = Fire and forget (pub/sub)
  • Send = I expect someone to do something with this message throw if no receiver / handler, local handlers are preferred over remote receivers. Doesn't wait for message to be handled. Can optionally wait for acknowledgment.
  • Invoke = Process this message immediately and wait for the handler to be called whether it's local or remote

    • For local handlers, this means the handler will get called inline using the current thread
    • For remote handlers, this is mainly used for RPC style where you will await a response and it feels as if the handler is called inline. Could wait for some sort of default completion message when called without a response type. So it wouldn't return until the remote handler has been run.
  • Schedule = Publish this message later. Doesn't seem like you'd want to throw if there wasn't a handler for this, but I guess you could check for a handler up front. So maybe there would be ScheduleSendAsync and SchedulePublishAsync.

The "delayed" or scheduled messaging is backed by the same outbox messaging durability in the cases where the underlying transport doesn't support the delayed messaging. Which to be honest, Wolverine isn't using anything native yet.

Call it a very crude Hangfire directly embedded into Wolverine. The timing isn't going to be super accurate, but good enough for the kind of timeout messages it's meant for. And if the outbox/inbox is applied, the scheduled messages happily outlive the process and there's work sharing between nodes.

ejsmith commented 1 year ago

I think I'd vote for a separate, advanced InvokeRemotelyAsync() (Today's SendAndWait()), or leave that alone as is. Again, you want minimal surprised when the API is called.

"Use handler configuration to control durability and parallelism" -- that's only applicable to locally handled messages though, and to be technical, it is a separate queue. For the sake of some local usage where ordering becomes important, you would want/need to assign message types to local queue names. There's already an early wolverine user that utilizes that.

Yes, you would configure your local handlers. In the use case where you are wanting local queues of work, then there would be local handlers for those messages. And you could control remote message handlers in the exact same way.

I was saying that you could use topic for controlling priority/ordering. I've just been trying to see if there was a way to reduce number of concepts / methods / choices for all of these things.

What I'd say is to make it easy (I'd argue it already is) to either configure local message handling by message type or let users have more control by assigning message types per queue.

Yeah, like you said at the beginning, this is all subjective. Just throwing out my 2 cents and giving my initial reactions to attempting to try out the library.

Also very likely you'd want to specify exact queue names when publishing locally if there's higher or lower message priority.

Yep, again, I was saying use topic for this instead of introducing the new concept of local named queues.

I think I'd like to consider breaking Advanced into Local and Remote to make it even more clear what's happening. That eliminates the user surprise, and would let us use InvokeAsync() semantics for remote invocation -- not that I'd recommend folks use that too awfully much.

I don't have as much experience as you, but I guess I don't really understand why it's so horrible to have the same concepts for local / remote message handling. I don't think I'd find it surprising at all, but again, I don't have as much experience as you. It's (obviously) your call.

Could you do an interface mockup of what you are thinking currently so we can see how it looks and figure out if we like it better than what you have currently and if it's worth the effort to change?

jeremydmiller commented 1 year ago

"Yep, again, I was saying use topic for this instead of introducing the new concept of local named queues." -- it's already in the library! Introducing "topics" for local messages would be a new concept

"I don't really understand why it's so horrible to have the same concepts for local / remote message handling." -- yeah, sorry, I don't get why you don't understand that's a potentially big deal. And my viewpoint is already based on early users who did get confused by local processing versus sending a message. Sending a remote message vs enqueuing something locally is a big difference in performance, troubleshooting, and just understanding how your system is behaving later.

jeremydmiller commented 1 year ago

Yeah, and for that matter, "topic" implies pub/sub to potentially multiple subscribers, and that's not how the local queues would ever work. To put a fork in it, I'm saying no to using the "topic" terminology against local queues -- which are actually queues by the way.

jeremydmiller commented 1 year ago

First cut:

public interface IMessageBus
{
    // as is
    Task InvokeAsync(object message, CancellationToken cancellation = default);

    // I don't particularly like this signature/name, but don't have better suggestions. There's the distinct possibility
    // that it's not just a query, so it's a lie to call it QueryAsync()
    Task<T?> InvokeAsync<T>(object message, CancellationToken cancellation = default);

     // In functionality, this is really what's today IMessagePublisher.SchedulePublishAsync
     ValueTask ScheduleAsync<T>(T message, DateTimeOffset executionTime);
     ValueTask ScheduleAsync<T>(T message, TimeSpan delay);

    // There must be a subscriber
    ValueTask SendAsync<T>(T message, DeliveryOptions? options = null);

    // Doesn't have to be a subscriber
    ValueTask PublishAsync<T>(T message, DeliveryOptions? options = null);

}

public interface ILocalNode
{
     // What today is EnqueueAsync()
     ValueTask PublishAsync<T>(T message);
     ValueTask PublishAsync<T>(T message, string workerQueueName);

      // Schedule local execution
     Task<Guid> ScheduleAsync<T>(T message, DateTimeOffset executionTime);
     Task<Guid> ScheduleAsync<T>(T message, TimeSpan delay);
}

public interface IExternalMessaging
{
    ValueTask SendToTopicAsync(string topicName, object message, DeliveryOptions? options = null);
    ValueTask SendToEndpointAsync(string endpointName, object message, DeliveryOptions? options = null);

    // SendAndWaitAsync *could* go to InvokeAsync, but I think that SendAndWait is more descriptive of what's really happening
    Task<Acknowledgement> SendAndWaitAsync(object message, CancellationToken cancellation = default,
        TimeSpan? timeout = null);

    Task<Acknowledgement> SendAndWaitAsync(Uri destination, object message, CancellationToken cancellation = default,
        TimeSpan? timeout = null);

    Task<Acknowledgement> SendAndWaitAsync(string endpointName, object message,
        CancellationToken cancellation = default, TimeSpan? timeout = null);

    // I could be argued in favor of putting these on IMessageBus, but I'm not enthusiastic about folks doing this very much
    // I try to discourage this in our own shop
    Task<T> RequestAsync<T>(object message, CancellationToken cancellation = default, TimeSpan? timeout = null)
        where T : class;

    Task<T> RequestAsync<T>(Uri destination, object message, CancellationToken cancellation = default,
        TimeSpan? timeout = null) where T : class;

    Task<T> RequestAsync<T>(string endpointName, object message, CancellationToken cancellation = default,
        TimeSpan? timeout = null) where T : class;
}
jeremydmiller commented 1 year ago

And oops, add this to the IMessageBus interface:

ILocalNode Local {get;}
IExternalMessaging External {get; }
ejsmith commented 1 year ago

Am I interpreting this correctly thinking that the core IMessageBus interface is mostly about the local mediator type usages and that I'd be using IExternalMessaging for anything that would be handled by external services? If not, I'm not sure when I would use one vs the other.

RE: RequestAsync<T> I think there is a good amount of people out there that use message systems for RPC as an alternative to something like GRPC. Obviously, you'd not want to use it for every part of your app, but it does seem like it would be somewhat common.

jeremydmiller commented 1 year ago

@ejsmith See the last comment. I missed a detail. One main interface, period. But gather IMessageBus.Local or IMessageBus.External/Remote. Just like Marten's IDocumentSession.Advanced or IDocumentSession.Events

"RE: RequestAsync I think there is a good amount of people out there that use message systems for RPC as an alternative to something like GRPC. Obviously, you'd not want to use it for every part of your app, but it does seem like it would be somewhat common." -- that doesn't resonate with me too much though.

ejsmith commented 1 year ago

Ok, so I'd primarily be using SendAsync and PublishAsync methods at the root for most things and they would work for local and external handlers?

What would InvokeAsync at the root do in the case of an external handler?

What would be the difference between calling ScheduleAsync vs Local.ScheduleAsync?

What is the difference between InvokeAsync<T> and External.RequestAsync<T>?

jeremydmiller commented 1 year ago

"What would InvokeAsync at the root do in the case of an external handler?" -- it wouldn't. InvokeAsync is always executing locally. It's effectively MediatR.

"What would be the difference between calling ScheduleAsync vs Local.ScheduleAsync?" -- I was trying to make that clear that that's scheduling a message to be published up above in the comments. Dependening on routing, that could be local, but most likely remote

"What is the difference between InvokeAsync and External.RequestAsync?" -- running it inline in the local process vs. making a request/reply message to an external node.

Is the "Invoke()" nomenclature really that problematic? You're the only person who's had any issue with that. It's consistent w/ MediatR as I recall, and probably why I chose it. Are you sure you're not bringing your own personal view of these kinds of tools to this discussion?

ejsmith commented 1 year ago

"What would InvokeAsync at the root do in the case of an external handler?" -- it wouldn't. InvokeAsync is always executing locally. It's effectively MediatR.

Ok, so as a new user, I'd need to figure out somehow that InvokeAsync isn't for me as a distributed messaging user or this would need to be moved into Local.

"What would be the difference between calling ScheduleAsync vs Local.ScheduleAsync?" -- I was trying to make that clear that that's scheduling a message to be published up above in the comments. Dependening on routing, that could be local, but most likely remote

I understand that it does a scheduled publish and you are saying it would work for local or remote. So the question is, when would I use Local.ScheduleAsync?

"What is the difference between InvokeAsync and External.RequestAsync?" -- running it inline in the local process vs. making a request/reply message to an external node.

Is the "Invoke()" nomenclature really that problematic? You're the only person who's had any issue with that. It's consistent w/ MediatR as I recall, and probably why I chose it. Are you sure you're not bringing your own personal view of these kinds of tools to this discussion?

I don't have any problem with Invoke. To me, invoke says that I am going to run the handler basically right now. My problem is that there is confusion in this framework because it does both local mediator style and distributed messaging. Now I have this Invoke method at the top that doesn't do anything for distributed messaging and I have a External.SendAndWaitAsync and a RequestAsync<T> that are doing similar things of sending out a message, executing the message handler and waiting for a reply back. Am I capable of understanding what your system does as it stands right now, of course, but this is an opinionated debate on whether this can be simplified or not.

jeremydmiller commented 1 year ago

"when would I use Local.ScheduleAsync?" -- when you want to schedule an action in the current application as opposed to scheduling publishing a message for later.

How about Local.ScheduleExecutionAsync() then?

This started with "why is there both ICommandBus and IMessagePublisher?" Where the first is local only operations, and IMessagePublisher is everything. You can't perfectly separate local and remote into too different services because you could easily want to do a mix of remote and local operations in a single outbox transaction.

We could rename ICommandBus to ILocalBus, and IMessagePublisher to IMessageBus, and maybe the there's an IMessageBus.Local that's mostly ILocalBus. I really don't want to use IMediator, as even the current ICommandBus actually does more than MediatR.

For scheduling, we could also disambiguate by saying "ScheduleLocalExecutionAsync()" vs "ScheduleDeliveryAsync()" or something.

At the end of this, what's your suggestion?

ejsmith commented 1 year ago

Well I think we have agreed to disagree, but my suggestion is something like this:

// works both local and distributed
public interface IMessageBus
{
    // There must be a subscriber, can ask for message delivery acknowledgement (just delivery, not that the handler has been run)
    ValueTask SendAsync<T>(T message, DeliveryOptions? options = null);

    // Doesn't have to be a subscriber
    ValueTask PublishAsync<T>(T message, DeliveryOptions? options = null);

    // Sends a message and waits for acknowledgment that the message has been handled (handler has run)
    Task<Acknowledgement> InvokeAsync(object message, CancellationToken cancellation = default, TimeSpan? timeout = null);
    Task<T?> InvokeAsync<T>(object message, CancellationToken cancellation = default, TimeSpan? timeout = null);

    // In functionality, this is really what's today IMessagePublisher.SchedulePublishAsync
    ValueTask ScheduleAsync<T>(T message, DateTimeOffset executionTime);
    ValueTask ScheduleAsync<T>(T message, TimeSpan delay);

    // tuck away some advanced options
    IAdvancedMessageBus Advanced { get; }
}

// inherits from IMessageBus so that it is mostly the same, but with some local only signatures.
// Also guarantees things will only ever be executed locally
public interface ILocalBus : IMessageBus
{
     // What today is EnqueueAsync()
     ValueTask SendAsync<T>(T message, string workerQueueName);
}

public interface IAdvancedMessageBus
{
    ValueTask SendToTopicAsync(string topicName, object message, DeliveryOptions? options = null);
    ValueTask SendToEndpointAsync(string endpointName, object message, DeliveryOptions? options = null);

    Task<Acknowledgement> InvokeEndpointAsync(string endpointName, object message, CancellationToken cancellation = default, TimeSpan? timeout = null);

    Task<T> InvokeAsync<T>(Uri destination, object message, CancellationToken cancellation = default, TimeSpan? timeout = null) where T : class;
    Task<T> InvokeEndpointAsync<T>(string endpointName, object message, CancellationToken cancellation = default, TimeSpan? timeout = null) where T : class;
}

I think your framework is the only one I know of that does Mediator style local message execution as well as distributed messaging. I personally feel like it's a VERY compelling value proposition to be able to start with a mediator style message based architecture and be able to easily graduate to distributed messaging where it makes sense with minimal (if any) changes.

ejsmith commented 1 year ago

The most common form of distributed processing is probably worker queues. With Wolverine, I could start with in process worker queues by sending messages to local handlers and using a durable inbox and even have multiple instances of my mononlith work share those since they'd be persisted and picked up by each instance of my app. Then I could eventually get to a point where doing that processing inside of my main application process is undesirable and easily graduate to that message being handled in a distributed out of process service.

To me, that is the kind of compelling use case that could make this framework become very popular.

jeremydmiller commented 1 year ago

After talking quite a bit to @oskardudycz off to the side and reading the comments here, I think y'all have convinced me to embrace the "wolverine as mediator" thing and streamline the API -- even if that means there's some potential confusion on whether or not the message is running locally or remotely. There will be some diagnostics on that of course.

Roughly, there'll be a new, single IMessageBus that's very close to @ejsmith 's last suggestion. Basic nomenclature:

And something for the rarely used features. Thinking of making you bounce through bus.EndpointFor(name/Uri).Publish/Schedule/Invoke()

The docs are going to be in awful state for just a little bit. As opposed to just being skimpy.

ejsmith commented 1 year ago

That's awesome @jeremydmiller! I really think this is going to simplify things with less concepts and consistency between local and distributed. I think Wolverine is going to have a very compelling story with being able to start with a simple message oriented monolith application with in process worker queues and then being able to very easily grow to a fully distributed application by picking and choosing which parts of the application need to be run as separate services. Thank you for listening! :-)

ejsmith commented 1 year ago

Also, I really like your EndpointFor idea.

ejsmith commented 1 year ago

I'm feeling lucky now and I'm going to push my luck a bit more. :grinning:

When I was saying that you could use topic for your local queues. Topics aren't just for pub/sub. Lots of messaging systems use topics as basically just a single file line for a set of messages. Convention for Wolverine and MassTransit is to create one topic per message type which means that each message type gets its own queue (or line). I can choose to put multiple message types into a single topic and then a subscription to that topic gets all of the various messages. Or I can subscribe to multiple topics in a single subscription. But basically, topic = queue of messages = 1 single file line. That's all I was saying for using topic to control queuing up messages to be processed locally. For local message handling, maybe by default all messages go into the default topic / queue. I could then use configuration to say messages of this type go into a topic / queue called blah and I could also control that on an individual Send call. The benefit of this approach is again just that there are less concepts and less differences between local and distributed messaging.

Alright, I will stop now. I am super happy about the direction you are going regardless of what you decide on this "topic" :wink:.

jeremydmiller commented 1 year ago

I'm still concerned that folks will shoot themselves in the foot over not understanding if something is local or remote, but we'll beat that w/ documentation and diagnostics (and hope).

"I can choose to put multiple message types into a single topic and then a subscription to that topic gets all of the various messages. "

You can already do that w/ Wolverine.

tl:dr --> topic != queue

The existing SendToEndpointAsync(name) already works for local queues by local queue name, which gives you the ability to direct messages to the exact local queue you'd want (i.e., "high" vs "low"). The SentToTopicAsync() thing is really meant for Rabbit MQ style topic patterns. I don't want anything to do with that kind of routing locally TBH. That smells like veering into Actor framework territory that I'm not prepared to deal with anytime soon. And there's nothing that's endpoint/queue-centric about the message handling in any sense. The main reason I'd use the topic routing would be to move around messages to stateful things, like if you had a sticky agent per tenant maybe.

And you do have the ability to setup subscribers to specific topics in Rabbit at least, while also having the ability to declare topic exchanges to get at messages by topic pattern matching.

To be technical, MT & Wolverine create a single queue for listening for each message type (if you opt into conventional routing) and a fanout exchange for sending via rabbit, or just a queue for SQS/Azure SB. I might be getting dragged into Rabbit MQ nomenclature here, but "topic" is just a different animal that isn't supported quite the same in different brokers. I don't want to conflate topic and queue here.

"But basically, topic = queue of messages = 1 single file line" -- yeah, that's not really true.

"For local message handling, maybe by default all messages go into the default topic / queue."

-- that's actually how it worked until just an hour ago when I moved it to using queue per message type by default, while retaining the ability to explicitly override that as desired:-)

"I could then use configuration to say messages of this type go into a topic / queue called blah and I could also control that on an individual Send call"

You could already do that w/ Wolverine, and could in the new model with the EndpointFor(name).Send(message) -- with the proviso that now you need to declare local queues upfront if they aren't discovered from conventions.

One way I want to vary from MassTransit in particular is to have routing rules expressed in the language of each underlying broker. I.e., don't try to make the abstraction over SQS or Azure Service Bus use Rabbit MQ's particular terminology like unique names.

I'm iffy on doing it this way for some internal reasons, but we could just make an explicit topic name be done through DeliveryOptions. I went the way I did w/ SendToTopicAsync() because it's discoverable where options in DeliveryOptions are not.

ejsmith commented 1 year ago

Alright, sounds good. I understand your concerns.

As far as SendToTopicAsync, I agree that there is value in discoverability vs tucking that into DeliveryOptions. It's just a matter of how often will people want to override the default topic on a per call basis? Also, by exposing that method, are you possibly steering people into thinking that is the way you should control the topic every time vs using convention / configuration? My personal opinion is that it should be tucked into DeliveryOptions and encourage people to use the conventions / configuration although I understand that there are cases where you would need to control it per call.

jeremydmiller commented 1 year ago

There's also an internals/perf concern too on that guy. I'm trying hard to eliminate runtime if/then logic, and putting it back on DeliveryOptions puts into adding more runtime steps in the "hot path". The routing is a wee bit different because a topic isn't valid on every sending endpoint. I'd have to think on that one a bit more. Not completely saying no here.

ejsmith commented 1 year ago

Could you possibly change the DeliveryOptions parameter to be a func that configures a given DeliveryOptions instance and then internally you pool the instances?

jeremydmiller commented 1 year ago

It's not that, the routing has to behave a little differently to validate that there really is at least one subscriber that accepts a topic. I know, micro-optimization, but I'm trying to do be that perf conscious throughout the pipeline

jeremydmiller commented 1 year ago

Hey folks, here's where it stands as of this very second:

https://github.com/JasperFx/wolverine/blob/main/src/Wolverine/IMessageBus.cs

The effort to boil down the public interface has taken way, way more work than I anticipated (brought more than a few problems to light, did help remove some hokiness in the code while adding a little bit of new hacky code to deal w/ later).

Have at it with any comments.

jeremydmiller commented 1 year ago

TBH, the ScheduleAsync() methods could go away and folks could use DeliveryOptions. It's just syntactic sugar at this point, though it wasn't originally.

ejsmith commented 1 year ago

Seems like they would be used commonly enough that it wkd be worth having them for discovery reasons.

jeremydmiller commented 1 year ago

I'm declaring success for now.

niemyjski commented 1 year ago

One thing I wanted to comment on was if timing is set on delivery options and you specify a timespan or date argument…. should we throw?Sent from my iPhoneOn Dec 12, 2022, at 11:55 AM, Jeremy D. Miller @.***> wrote: Closed #104 as completed.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: @.***>

jeremydmiller commented 1 year ago

What do you mean? If the DateTimeOffset is in the past, you just send it immediately

niemyjski commented 1 year ago

What do you mean? If the DateTimeOffset is in the past, you just send it immediately

I was referring to ScheduleAsync here: https://github.com/JasperFx/wolverine/blob/main/src/Wolverine/IMessageBus.cs#L46-L47 The delay could already be set on delivery options passed into ScheduleAsync. I'm assuming it would always be overwritten.