awslabs / aws-dotnet-messaging

An AWS-native framework that simplifies the development of .NET message processing applications that use AWS services, such as SQS, SNS, and EventBridge.
https://awslabs.github.io/aws-dotnet-messaging/
Apache License 2.0
81 stars 15 forks source link

Publish to queue? #89

Open CallumHibbert opened 11 months ago

CallumHibbert commented 11 months ago

System design with messaging must be done carefully and should follow fairly strict principles. One such principle is the difference between commands and events.

Put very simply, a command is an instruction to do something and should be sent (not published) to a specific recipient. Sending to a specific recipient infers using a queue. In AWS world this means using SQS.

An event is a notification that something happened and should be published (not sent) with the publisher not really aware of the subscribers. Publishing infers using a topic. In AWS world this means SNS or possibly Event Bridge.

There is a clear distinction between "send" vs "publish", what those operations do and how they work.

These principles are really important to achieve good system design and the current API doesn't follow these. In the context of the above principles, an "SQS publisher" (as provided by the library) does not make sense (you shouldn't "publish" to a "queue").

Going directly to a proposed alternative, consider one interface with differing methods e.g.

public interface IBus
{
    Task PublishAsync<T>(T event, CancellationToken token = default); // SNS or EventBridge Transport

    Task SendAsync<T>(T command, CancellationToken token = default); // SQS Transport only
}

Or two different interfaces e.g.

// SNS or EventBridge Transport
public interface IMessagePublisher
{
    Task PublishAsync<T>(T event, CancellationToken token = default);
}

// SQS Transport only
public interface IMessageSender
{
    Task SendAsync<T>(T command, CancellationToken token = default);
}

This encourages consumers to follow a system design better aligned with correct messaging principles.

ashovlin commented 9 months ago

Thanks for the feedback!

We currently have a generic IMessagePublisher which is implemented by MessageRoutingPublisher. This can be used to publish/send to any of the supported services, the service and destination is looked up at runtime based on how the message type is mapped. https://github.com/awslabs/aws-dotnet-messaging/blob/2ec157f7099c091aaf07cb89059f574e3abcca26/src/AWS.Messaging/IMessagePublisher.cs#L25

We also have the service-specific publishers such as ISQSPublisher implemented by SQSPublisher. These expose additional, service-specific options, though again all through PublishAsync. https://github.com/awslabs/aws-dotnet-messaging/blob/2ec157f7099c091aaf07cb89059f574e3abcca26/src/AWS.Messaging/Publishers/SQS/ISQSPublisher.cs#L19


I think we're open to separating the service-specific interfaces and implementations like how you suggested:

But we're more on the fence for adjusting the generic publisher. I think we want to keep it overall since it's simpler if one doesn't need to take advance of service-specific features. I'm curious about your thoughts on:

  1. Do you think it's okay leaving a consolidated method here, if we corrected the service-specific publishers?
  2. If the generic IMessagePublisher did expose both a PublishAsync and SendAsync, how should it behave if one tried to call PublishAsync with a message type that is mapped to the "wrong" service? Throw an exception at runtime? Perhaps succeed anyway, but log a warning?
ashovlin commented 9 months ago

We shipped renaming of the service-specific publishers today 0.2.0-beta via #101. The names now match the proposal above

Let us know if you have any other feedback on design or naming, thanks!

CallumHibbert commented 9 months ago

Hi @ashovlin

Sorry for the delayed feedback. I started a new job recently which has taken all my attention. I did read your first comments but forgot to get back to you.

I like the changes you have made, differentiating the Send and Publish methods is really important because those operations are very different actions with very different architectural semantics.

My only remaining thought is that, looking at this purely from an consumer/outsider's perpective, it is odd to see a SendAsync method on a ...Publisher (i.e. ISQSPublisher.SendAsync) especially when other other "publishers" have a Publish method (e.g. SNSPublisher.PublishAsync. From the consumer's view, it might make more sense for a "sender" to be called exactly that e.g. SqsSender to give SQSSender.SendAsync.

One other (unrelated) piece of feedback is that your might want to run FxCop (or what we call "Code Analysis" these days) over the code base because the .NET coding standards state that acronyms of three or more letters should be pascal cased. See: https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/capitalization-conventions#capitalization-rules-for-identifiers

In this code base this would mean to use Sqs and Sns instead of SQS and SNS respectively, giving SqsPublisher (or maybe SqsSender) and SnsPublisher . If you've not ran FxCop/Code Analysis yet then it might pick up one or two more issues (e.g. marking your code CLS compliant). Some of these modifications may be breaking changes and you'll want to fix those before you ship version 1.0.

Thanks very much for considering and actioning the feedback.

brendonparker commented 5 months ago

We currently have a generic IMessagePublisher which is implemented by MessageRoutingPublisher. This can be used to publish/send to any of the supported services, the service and destination is looked up at runtime based on how the message type is mapped.

For what it is worth, I really like this. Please don’t change it by having Publish and Send as different methods on that interface.

mgmccarthy commented 2 months ago

Adding support for both of @CallumHibbert's comments.

There are other frameworks I've worked with that support these types of naming conventions and very clearly delineate the logical as physical differences between commands and events.

Enterprise Integration Patterns:

NServiceBus Message Bus:

MassTransit

mgmccarthy commented 1 month ago

Any chance you'd be open to a pull request to explore the API changes mentioned above in terms of sending a message to a queue vs. publishing a message to a topic?