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

Support for UniqueId on QueueEntryOptions (with MessageBus support) #268

Merged
merged 24 commits into from
Jan 24, 2022

Conversation

jcono
Copy link
Contributor

@jcono jcono commented Dec 13, 2021

Driven by the need to have duplicate detection functionality Azure Service Bus (see FoundatioFx/Foundatio.AzureServiceBus#39).

Adds a UniqueId property to the QueueEntryOptions class and supports passing options publishing a message to an IMessageBus.


namespace Foundatio.Messaging {
public interface IMessagePublisher {
Task PublishAsync(Type messageType, object message, TimeSpan? delay = null, CancellationToken cancellationToken = default);
Task PublishAsync(Type messageType, object message, TimeSpan? delay = null, QueueEntryOptions options = null, CancellationToken cancellationToken = default);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we passing in a QueueEntryOptions into a messaging implementation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is a breaking change. We can't really change this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did wonder about this. Does it need a MessageEntryOptions instead then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, either there is some sort of breaking change or we go back to having the message object itself implement an known interface in order to provide the unique message id (but you guys suggested otherwise in FoundatioFx/Foundatio.AzureServiceBus#39 unless I misunderstood your intent).

Unless you can think of another option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@niemyjski and @ejsmith

Any steer on what you would like to do with this?

Copy link
Contributor Author

@jcono jcono Jan 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I primarily need it for messaging but I do use both. I've had to essentially fork your code to be able to use it for our purposes (with the duplicate detection) but I was hoping it was useful enough to incorporate into the packages and consume them that way so I didn't diverge over time and create a maintenance issue.

Happy to take your steer on what to do? Go back to the interface? Wait till you've had time to consolidate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am hoping to get time to work on the consolidation soon, but I think it the meantime, if you wanted to submit a PR that makes the IMessagePublisher more like the queue one where we have MessageOptions like you suggested as a parameter similar to how we have QueueEntryOptions on queues now. That way we aren't adding a parameter to the contract for a single feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks. I'll try and get that together over the next few days (time permitting).

Copy link
Contributor Author

@jcono jcono Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ejsmith I've updated this PR (and the ones in the other repositories) with a MessageOptions instead. Let me know what you guys think.

The other PR's are:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's looking good. Thanks for your efforts with this!

Copy link
Contributor

@ejsmith ejsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ejsmith ejsmith merged commit 846542c into FoundatioFx:master Jan 24, 2022
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.

None yet

3 participants