Particular / NServiceBus.AzureServiceBus

The Azure ServiceBus Transport
https://docs.particular.net/nservicebus/azure-service-bus/
Other
13 stars 17 forks source link

The bundles feature for the ForwardingTopology is imposing unnecessary limitations on the endpoints #521

Closed SeanFeldman closed 6 years ago

SeanFeldman commented 7 years ago

The original design of the ForwardingTopology was based on the ASB documentation stating that: image

The documentation is now refreshed and says that number of concurrent connections is

Capped by the limit of concurrent connections per namespace.

Which is

NetMessaging: 1,000 / AMQP: 5,000

With this, we'll need to

  1. Deprecate the API for bundles with warning, treat as an errorin the next major, and remove in next major+1.
  2. Change the default behavior to have a single topic only. Could cause message loss (see below), issue #524
  3. Update documentation (samples?) and guidance.
  4. Review configuration API for the shared topic (BundlePrefix(string) today).

Related issue: #522

Connects to https://github.com/Particular/NServiceBus.AzureServiceBus/issues/587

SeanFeldman commented 7 years ago

Wanted to follow the same approach the core is taking for obsoletes (we've used it inconsistently in this transports) and discussed it with @bording. During our discussion, going through the reasons for the change, one scenario was touched that we haven't discussed.

Assuming the defaults are changing and we create a single bundle (bundle-1 by default rather than two), when upgrading subscribers first, there's a chance of loosing messages if subscribers along with upgrade of the transport also introduced a change in the events they subscribe to.

image

Consider Subscriber2 upgraded to operate with a single topic in a bundle and subscribing to an EventD. Whenever Publisher would publish to the bundle-2 topic, the message would not be delivered to the Subscriber2 and be lost.

Introducing this change in a minor with warnings and upgrade guides could still lead to this compatibility break. If part of the system is on 7.x and another part is upgraded to 7.x+1, it's not clear from the minor upgrade that topology could have a breaking change and requires upgrade of all the endpoints.

Assuming this change is left for a major version, it would still not address the compatibility issue. Also, this would make this worse as more customers would adopt this topology that was only introduced in version 7 of the transport.

@Particular/azure-maintainers please weigh in on this issue.

danielmarbach commented 7 years ago

I'd say we can do the following:

  1. Deprecate all config APIs
  2. Update documentation (samples?) and guidance.
  3. Review configuration API for the shared topic

Still create two bundles by default. Enable an Opt-in for one bundle, Write migration and potential failure scenario guidelines and slowly transition over to single bundle approach.

Another option would be to leave the two bundles but do not allow to configure it. We'd have to deal with this complexity for a long time but I don't think it is such a big issue.

What are the downside of leaving two bundles forever? Remember we also have the .NET Standard compliance issue around which eventually could give us the opportunity to create a new trimmed down transport in the future.

yvesgoeleven commented 7 years ago

The downsides are:

yvesgoeleven commented 7 years ago

Imo we should add a startup check to validate that the number of entities configured actually matches the number of entities in the bundle (and throw if it is not the case). E.g. user could configure 5 entities first, then 2 on future deployment and get into this scenario as well.

SeanFeldman commented 7 years ago

Like the idea @yvesgoeleven Especially since we've got the possibility of miss-configuration already in place that can cause the same situation as described above. The startup check would validate a single topic with a bundle prefix is found.

danielmarbach commented 7 years ago

And therefore make the startup even slower with that additional check?

SeanFeldman commented 7 years ago

I believe this is a different issue because its already in the transport. Has nothing to do with the number of topics in the bundle from a connectivity point of view, but with missconfigured endpoints for number of topics.

SeanFeldman commented 7 years ago

@danielmarbach the discussion about the check is moving to #524. Let's keep this issue focused on the future of the API.

Deprecate all config APIs Update documentation (samples?) and guidance. Review configuration API for the shared topic

Already listed in actions to take in the general description as items 1, 3, and 4.

Still create two bundles by default. Enable an Opt-in for one bundle, Write migration and potential failure scenario guidelines and slowly transition over to single bundle approach.

Opt-in for one bundle - do you mean to have a configuration that is different from the configurations we've already got? If yes, then 👍 for the above.

Another option would be to leave the two bundles but do not allow to configure it. We'd have to deal with this complexity for a long time but I don't think it is such a big issue.

I'd rather have this achieved via #524 and guidance + obsolete on the existing API.

What are the downside of leaving two bundles forever? Remember we also have the .NET Standard compliance issue around which eventually could give us the opportunity to create a new trimmed down transport in the future.

Would not count on the .NET Standard client in the near future - remember it will not have feature parity for quite a while 😉

mikegore1000 commented 7 years ago

Do any stats exist to show the impact of using a single topic for the topology? Based on the doc: https://docs.microsoft.com/en-us/azure/service-bus-messaging/service-bus-performance-improvements

It looks like if we wanted to increase throughput we'd want to use more entities. If you are only allowing one bundle, doesn't this constrain throughput to how quick ASB can auto-forward from the topic to the per endpoint queue?

We are using premium messaging, but the only stats I've seen so far are here: https://blogs.msdn.microsoft.com/servicebus/2016/07/18/premium-messaging-how-fast-is-it/

These don't show if throughput increases or decreases with more messaging entities though, just MUs. I want to ensure that the topology will be scalable moving forward. At present we have a small about of endpoints on ASB and it's just for queues so changing topology if it makes sense now is low risk.

While we could increase MUs this does have a cost and if it's more cost effective to use multiple topics, that seems to make more sense.

SeanFeldman commented 7 years ago

@mikegore1000 increased number of entities in the article you pointed to is referring to the client library communicating with the broker. In that scenario, indeed, increased number of clients and factories improves overall throughput on an entity. With ForwardingTopology and bundles auto-forwarding operation is on the server side and client side rules do not apply. Given there's minimal latency and high throughput, this should not have a significant impact.

Unfortunately, there's no documentation on performance best practices for server side auto-forwarding. You could raise an issue with the ASB team to provide that documentation.

yvesgoeleven commented 7 years ago

@mikegore1000 as far as we know, the number of entities does not matter (anymore?), the limits are at MU level for premium or namespace level in standard, so adding more entities from the same namespace doesn't make a lot of sense.

mikegore1000 commented 7 years ago

@yvesgoeleven Agree that if there are no limits at an entity level then adding additional entities per namespace doesn't make much sense at all. I had interpreted the docs to say that adding more entities irrespective of client settings improved throughput rather than it being specifically a client concern.

Based on this we'll just make sure to use a single bundle topic.

I asked MS this question for them to verify a few hours ago, so I'll let you know either way what they say. Sounds like this may take a few days though.

yvesgoeleven commented 7 years ago

@mikegore1000 from our performance testing the number of connections to the broker (aka messagingfactory instances) is what mattered most.

SeanFeldman commented 7 years ago

I asked MS this question for them to verify a few hours ago

@mikegore1000 could you provide a link to the issue you've raised? Thanks.

mikegore1000 commented 7 years ago

@SeanFeldman This is via email, so no link unfortunately. I'll update as soon as I hear anything.

mikegore1000 commented 7 years ago

@SeanFeldman Still not heard anything, so sent a chase up. However, I did find the following stack overflow thread with a comment from Clemens Vasters that seems to suggest that number of topics doesn't matter: https://stackoverflow.com/questions/41759502/azure-service-bus-multiple-topics-vs-filtered-topic/41760188#41760188

mikegore1000 commented 7 years ago

@SeanFeldman Just heard back from MS and they have confirmed that everything aligns with the stack overflow comment I posted above. As soon as I found that article I assumed that would be the case based on Clemens' role. Thanks for the support with this!

SeanFeldman commented 7 years ago

More than welcomed. I've raised an issue with ASB team to hopefully see this guidance added to the official documentation.

danielmarbach commented 6 years ago

We moved back to one topic per bundle. Closing this.