Azure-sdk-for-net: [Discussion] Service Bus - Track 2 - Message Settlement Location

[JoshLove-msft]

_The intent of this issue is to solicit feedback from the community in advance of the GA of the next major version of the Service Bus library (which will follow several Preview releases).
As a reference, the .NET Azure SDK guidelines can be found here: https://azure.github.io/azure-sdk/dotnet_introduction.html
Terminology:
Track 0 refers to the WindowsAzure.ServiceBus library
Track 1 refers to the Microsoft.Azure.ServiceBus library
Track 2 refers to the new library, Azure.Messaging.ServiceBus, we are working on releasing._

Background
Settlement refers to the operations that can be used to settle a message. These are Complete, Abandon, Defer, and Dead Letter. In version 4 of the Service Bus library, the settlement methods are located directly on the client types. In an earlier version of the library, the settlement methods were on the message. We would like to explore the pros and cons of placing the settlement methods in each place.

Lock token overloads
We are planning to include both lock token and message overloads for settlement. The lock token overload is useful for cases where the user may not want to hold onto the entire message before settling. By putting these on the receiver, we would be able to put the overloads on the same type.
```c#
var client = new ServiceBusClient("connectionString");
ServiceBusReceiver receiver = client.GetReceiver("myQueue");
ServiceBusReceivedMessage message = await receiver.ReceiveAsync();
// message overload
await receiver.CompleteAsync(message);
// lock token overload
await receiver.CompleteAsync(message.LockToken);

If we put the settlement methods on the message, we would need to split out the location of the lock token overload:

```c#
await message.CompleteAsync();
await receiver.CompleteAsync(message.LockToken);

Batch overloads
Similar to the point about lock token overloads, batch versions of the settlement methods would have to be placed on the receiver. So if we put the single message settlement on the message itself we would end up with something like this:
c# IList<ServiceBusReceivedMessage> messages = await receiver.ReceiveBatchAsync(maximumMessageCount: 10); // using the single message settlement foreach (var message in messages) { await message.CompleteAsync(); } // or with the batch version await receiver.CompleteAsync(messages);

Other considerations
Another issue to consider is how users might think about a settlement method in the context of the ServiceBusReceivedMessage type. Might they think that they are performing a local operation rather than a service call, since they are not calling the method from the receiver type? Also, separating out the settlement methods from the receiver might lead users to believe that they can safely dispose of the receiver before settling the message. This isn't the case as the message would still be using the receiver's underlying link to do the message settlement.

One argument in favor of settlement methods being placed on the message, is that it allows for somewhat simpler user code. For instance, users wouldn't need to pass both the receiver and message along to any method that they write for processing a received message.

Conclusion
In spite of some of the simplifying benefits that placing settlement methods on the message allows, we are planning to put the settlement methods on the receiver. This allows for a consistent API when considering the lock token and batch settlement methods. It is also more consistent with the other new Azure SDK libraries, where service operations are generally located on the client types.

[JoshLove-msft]

Adding some community members who have recently submitted issues/PRs for Service Bus 😃

/cc @nemakam @SeanFeldman @Einarsson @brandondahler @broloaugh @ankrause @GrimGadget @ShaneCourtrille @Pucmaster @Bnjmn83

[SeanFeldman]

Nitpicking: for consistency, let's call "version 4 of the Service Bus library" with the name that is used everywhere, Track 1. There was also a version 4 of the SB library in Track 0. We don't want to add confusion.

[SeanFeldman]

Track 0 had settlement operations on the BrokeredMessage.
Track 1 had settlement operations on the clients (receivers).

After using both variants my conclusion that the desire to simplify things should not blind the users. While the API _might_ be interpreted as easier to grasp, there's the undeniable need to understand how the service works. A Message is never settled on the client. A message is a loaner copy given to the client. The broker is responsible for the settlement and it doesn't care about message ID or any other property. Except for lock token.

In addition to that, adding the settlement responsibility to the ServiceBusReceivedMessage will turn it from a simple DTO to a type with cross-cutting concerns it shouldn't have.

The arguments against having settlement methods on the message exceed the negatives associated with the settlement methods on the clients. Personally, I fully agree with the conclusion.

[JoshLove-msft]

Track 0 had settlement operations on the BrokeredMessage.
Track 1 had settlement operations on the clients (receivers).

After using both variants my conclusion that the desire to simplify things should not blind the users. While the API _might_ be interpreted as easier to grasp, there's the undeniable need to understand how the service works. A Message is never settled on the client. A message is a loaner copy given to the client. The broker is responsible for the settlement and it doesn't care about message ID or any other property. Except for lock token.

In addition to that, adding the settlement responsibility to the ServiceBusReceivedMessage will turn it from a simple DTO to a type with cross-cutting concerns it shouldn't have.

The arguments against having settlement methods on the message exceed the negatives associated with the settlement methods on the clients. Personally, I fully agree with the conclusion.

Great to hear! And thanks for providing a nice argument in favor of settlement methods on the client.

[JoshLove-msft]

Nitpicking: for consistency, let's call "version 4 of the Service Bus library" with the name that is used everywhere, Track 1. There was also a version 4 of the SB library in Track 0. We don't want to add confusion.

We were avoiding Track 0/1/2 as we didn't expect this to be widely known, but you raise a good point that even using version numbers won't suffice. It might be best to just include the nomenclature in the blurb that we put at the top of each of these issues.

[SeanFeldman]

We were avoiding Track 0/1/2 as we didn't expect this to be widely known

Good one 🤣

[JoshLove-msft]

We were avoiding Track 0/1/2 as we didn't expect this to be widely known

Good one 🤣

Haha, fair point.

[SeanFeldman]

I guess the only thing that I've coined was Track 0 to refer to the closed source Service Bus SDK.

[Einarsson]

I strongly agree with all the reasoning stated above. Conclusion +1

[ShivangiReja]

/cc: @danielmarbach

[danielmarbach]

I'm also aligned with the reasonings presented here. Also 👍 for leaving the message as much as possible a data transfer object.

[danielmarbach]

@JoshLove-msft @ShivangiReja I stumbled now over ProcessSessionMessageEventArgs which is an event args but still has session related settlement operations. What do you think should to outcome of this discussion mean for the event args? Shouldn't then the event args also be "just a DTO type" and not have any additional behavior or is this a reasonable exception due to the fact that the event args have to get modified and the session is hidden by design?

[JoshLove-msft]

@JoshLove-msft @ShivangiReja I stumbled now over ProcessSessionMessageEventArgs which is an event args but still has session related settlement operations. What do you think should to outcome of this discussion mean for the event args? Shouldn't then the event args also be "just a DTO type" and not have any additional behavior or is this a reasonable exception due to the fact that the event args have to get modified and the session is hidden by design?

That is an interesting point. Since messages need to be settled on the link that they were received on, if we didn't expose the settlement methods in the event args, and instead just put these methods directly on the processor, we would have to keep a synchronized internal map of message to receive link and settle on the correct link. It would be possible to do, but it seemed more transparent to include this within the event args as the settlement methods you are accessing will be automatically associated with the link the message was received on. We don't have the same issue for the Receiver, since there will only ever be one receive link.

[danielmarbach]

I share the conclusion. I do agree the current design makes sense. The only other option apart from the one you mentioned I see would be to expose a behavior component as a property on the event args which then users have to transitively navigate into. That though wouldn't allow to set the property on the args and in general I'm not a huge fan of transitive dependency train wrecks

[SeanFeldman]

other option apart from the one you mentioned I see would be to expose a behavior component as a property on the event args which then users have to transitively navigate into.

That's the direction I was hoping the team will take as well. That's also the direction Track 1 was heading into.

Just to point out - this issue is now discussing two topics, settlement on Message vs client and Message settlement when using the processor. For future discussions, it would be better to split those and cross-link if needed.

[axisc]

my 2 cents - There is value in simplifying the client code by removing the overloads of the settlement methods that take the lock token. Some of the advantages are

• Simpler day 1 scenarios (I do occasionally find myself in the day 1 developer shoes 👍 and intuitively look to complete the message itself).
• Abstracting the service behavior and implementation.