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

Publish to queue? #89

Open
CallumHibbert opened this issue Dec 17, 2023 · 3 comments
Open

Publish to queue? #89

CallumHibbert opened this issue Dec 17, 2023 · 3 comments
Assignees
Labels
feature-request A feature should be added or improved. needs-review p2 This is a standard priority issue

Comments

@CallumHibbert
Copy link

CallumHibbert commented Dec 17, 2023

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.

@ashishdhingra ashishdhingra added feature-request A feature should be added or improved. needs-review labels Jan 12, 2024
@ashovlin ashovlin self-assigned this Feb 9, 2024
@ashovlin
Copy link
Contributor

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.

Task PublishAsync<T>(T message, CancellationToken token = default);

We also have the service-specific publishers such as ISQSPublisher implemented by SQSPublisher. These expose additional, service-specific options, though again all through PublishAsync.

Task PublishAsync<T>(T message, SQSOptions? sqsOptions, CancellationToken token = default);


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

  • ISQSPublisher/SQSPublisher -> SendAsync
  • ISNSPublisher/SNSPublisher -> PublishAsync
  • IEventBridgePublisher/EventBridgePublisher -> PublishAsync

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
Copy link
Contributor

ashovlin commented Mar 8, 2024

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

  • ISQSPublisher/SQSPublisher -> SendAsync
  • ISNSPublisher/SNSPublisher -> PublishAsync
  • IEventBridgePublisher/EventBridgePublisher -> PublishAsync

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

@CallumHibbert
Copy link
Author

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.

@ashishdhingra ashishdhingra added p2 This is a standard priority issue needs-review and removed closing-soon labels Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. needs-review p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests

3 participants