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

feat: extract message routing from the message pump #141

Merged

Conversation

stijnmoreels
Copy link
Member

@stijnmoreels stijnmoreels commented Dec 16, 2020

Extracts the message routing that includes the selection of the correct registered message handler for the incoming message to be processed.

Because the MessagePump and by result the AzureServiceBusMessagePump already contained methods like ProcessMessageAsync which handles this message routing, we can only mark them as obsolete but can't change them in any way.

This PR includes the introduction of the IMessageRouter and IAzureServiceBusMessageRouter which is the exact functionality of this message routing. The AzureServiceBusMessagePump already uses this router so the existing tests will suffice to make sure that this routing is still valid. An extra interface for the Service Bus was required, because the message handling required specific Service Bus operations calls and the original Service Bus message upon fallback.

Implementation of POC: https://github.com/stijnmoreels/arcus.messaging.azurefunctions
Relates to #127

@stijnmoreels
Copy link
Member Author

stijnmoreels commented Dec 16, 2020

Currently WIP, TODO list:

  • Message routing implementation
  • Unit tests to proof message routing implementation
  • Feature docs update to specify how consumers can use this routing in their own projects (ex. Azure Functions).

@stijnmoreels stijnmoreels changed the title Feature - extract message routing from the message pump 🚧 Feature - extract message routing from the message pump Dec 16, 2020
@stijnmoreels
Copy link
Member Author

I've not updated the MessagePump (yet) with using the IMessageRouter instead, because that would mean that we require such a registration in the IServiceProvider; which would mean that we fail when there's no such registration. And this is actually a breaking change, so I've let the protected methods on the pump as-is but marked them all obsolete until we can safely in a new major version break this.

@stijnmoreels stijnmoreels changed the title 🚧 Feature - extract message routing from the message pump Feature - extract message routing from the message pump Dec 18, 2020
@stijnmoreels stijnmoreels marked this pull request as ready for review December 18, 2020 14:05
@tomkerkhove tomkerkhove removed their assignment Feb 8, 2021
Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>
@stijnmoreels
Copy link
Member Author

OK, after reading all the remaining comments, there's only 2 that remains unsolved, and that's the mentioning of the .AddMessageRouting in the Customization feature docs (which we will discuss offline).

And the question how this extraction should be called 'Router' or 'Processor' or ... (honestly, if it were 'Processor' it would be confusing in combination with the handlers. "Process" <> "Handle" message?)

@tomkerkhove
Copy link
Contributor

Let's stick with Router then.

OK, after reading all the remaining comments, there's only 2 that remains unsolved, and that's the mentioning of the .AddMessageRouting in the Customization feature docs (which we will discuss offline).

What is this topic again?

@stijnmoreels
Copy link
Member Author

Let's stick with Router then.

OK, after reading all the remaining comments, there's only 2 that remains unsolved, and that's the mentioning of the .AddMessageRouting in the Customization feature docs (which we will discuss offline).

What is this topic again?

Described here: https://github.com/arcus-azure/arcus.messaging/pull/141/files#diff-a29b66ed26917dda14cce134994c1fd15605f0ecfbc72dd180eb355be4ee0b7cR71
Under the topic Extending the Message Router
Altough it seems reasonable to mention this if we're talking about extending and customization. It's already an advanced topic.

@tomkerkhove
Copy link
Contributor

Let's stick with Router then.

OK, after reading all the remaining comments, there's only 2 that remains unsolved, and that's the mentioning of the .AddMessageRouting in the Customization feature docs (which we will discuss offline).

What is this topic again?

Described here: #141 (files)
Under the topic Extending the Message Router
Altough it seems reasonable to mention this if we're talking about extending and customization. It's already an advanced topic.

Sure thing, but it's pending a reply from you, no?

@tomkerkhove tomkerkhove removed their assignment Feb 22, 2021
@stijnmoreels
Copy link
Member Author

Let's stick with Router then.

OK, after reading all the remaining comments, there's only 2 that remains unsolved, and that's the mentioning of the .AddMessageRouting in the Customization feature docs (which we will discuss offline).

What is this topic again?

Described here: #141 (files)
Under the topic Extending the Message Router
Altough it seems reasonable to mention this if we're talking about extending and customization. It's already an advanced topic.

Sure thing, but it's pending a reply from you, no?

No, it's anwsered here: #141 (comment), but can be disussed offline.

@stijnmoreels stijnmoreels changed the title Feature - extract message routing from the message pump feat: extract message routing from the message pump Apr 8, 2021
@stijnmoreels
Copy link
Member Author

Hm then I'm confused - Do I always have to call AddMessageRouting or not? Because you've mentioned that the message pump registration handles it for us; or is it only for Azure Service Bus?

Yes, we have to call it always, but it's being called behind the scenes with the Azure Service Bus message pump registration.

This was the latest awnsered question. But maybe you'll need to revisit the PR to know what's going on again 😄 .

@tomkerkhove
Copy link
Contributor

Super helpful, thanks!

@tomkerkhove
Copy link
Contributor

I see the PR closes the issue, did we add the handler for Functions as well? Think I saw it but want to be sure.

@tomkerkhove tomkerkhove removed their assignment Apr 12, 2021
@stijnmoreels
Copy link
Member Author

I see the PR closes the issue, did we add the handler for Functions as well? Think I saw it but want to be sure.

We can include that in #154 I guess. But I'll change it bc it's technically indeed not completely done. ☺️

@stijnmoreels stijnmoreels enabled auto-merge (squash) April 13, 2021 05:36
@stijnmoreels stijnmoreels merged commit cd036c9 into arcus-azure:master Apr 13, 2021
@stijnmoreels stijnmoreels deleted the feature/extract-message-routing branch April 13, 2021 05:44
@stijnmoreels stijnmoreels added this to the v1.0.0 milestone Jul 9, 2021
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

2 participants