-
Notifications
You must be signed in to change notification settings - Fork 300
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
improve direct routing topology warning #2262
Conversation
@Particular/rabbitmq-transport-maintainers @Particular/docs-maintainers please review. |
LGTM. @Particular/rabbitmq-transport-maintainers ? |
@@ -54,7 +54,7 @@ Adjust the conventions for exchange name and routing key by using the overload: | |||
|
|||
snippet:rabbitmq-config-usedirectroutingtopologywithcustomconventions | |||
|
|||
WARNING: The direct routing topology is nondeterministic for message types with "non-system" interfaces in their inheritance hierarchy. For deterministic routing of these message types, the conventional routing topology must be used. A "non-system" interface is any which is not contained in a .NET Framework assembly (any assembly signed with the same public key as mscorlib), and is not one of the [marker interfaces](/nservicebus/messaging/messages-events-commands.md#defining-messages-marker-interfaces). | |||
WARNING: In some cases, the direct routing topology may not deliver message types with "non-system" interfaces in their inheritance hierarchy. A "non-system" interface is any interface which is not contained in a .NET Framework assembly (any assembly signed with the same public key as mscorlib), and is not one of the [marker interfaces](/nservicebus/messaging/messages-events-commands.md#defining-messages-marker-interfaces). When using the direct routing topology, messsage types must not inherit from "non-system" interfaces in order to guarantee delivery. To guarantee delivery of message types which inherit from non-system interfaces, the conventional routing topology must be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording of the last sentence sounds a bit strange since, it is repeating the last three words of the previous sentence.
Perhaps "The conventional routing topology must be used to guarantee delivery of message types which inherit from non-system interfaces." instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it doesn't read well. Maybe that phrase could be removed from the preceding sentence:
When using the direct routing topology, message types must not inherit from "non-system" interfaces. To guarantee delivery of message types which inherit from non-system interfaces, the conventional routing topology must be used.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be handled on our side with detecting the case and throwing an exception? if not can it be handled on the users side by verifying via reflection and a unit test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SimonCropp detecting it and throwing an exception is something we've considered but it would be a breaking change for wire compatibility. It's certainly something to consider when trying to answer Particular/NServiceBus.RabbitMQ#270
@adamralph r u missing the full "fixes" link in the description? |
@SimonCropp oops! Yes, thanks. Corrected. |
af53583
to
37ebfa4
Compare
@bording I pushed a fixup! commit with the change we discussed. @Particular/docs-maintainers if you want that squashed before merging, just let me know. |
Thanks guys |
fixes Particular/NServiceBus.RabbitMQ#274