[WIP] Add the option to modify properties when abandoning a message #646
Conversation
This is useful to store exception details when abandoning a message and still keeping the convenience of registering a message or session handler with AutoComplete = true
test/Microsoft.Azure.ServiceBus.UnitTests/API/ApiApprovals.ApproveAzureServiceBus.approved.txt
Outdated
Show resolved
Hide resolved
| } | ||
| catch (Exception exception) | ||
| { | ||
| MessagingEventSource.Log.MessageReceiverPumpUserCallbackException(this.clientId, message, exception); |
There was a problem hiding this comment.
What this exception will look like? Will it contain enough info to understand that it's the user provided callback that has failed?
There was a problem hiding this comment.
At this point, the user callback threw an exception so the pump is abandoning the message. We let the user execute a second callback to return a dictionary of properties to modify on the message being abandoned, most likely to store details about the exception on the message itself. If that second callback fails, we don't want to fail abandoning the message so we just ignore the properties to modify and pass null (the default value). I'm not sure what else could be done, did you have something in mind?
There was a problem hiding this comment.
Logging that exception? My thought is the following:
Processing callback fails, message is going to be abandoned. Properties modifier callback is failing, the same log message will be made with a different exception, using MessagingEventSource.Log.MessageReceiverPumpUserCallbackException() variant. Perhaps it should be a more specific Log method to distinguish between callbacks? That would suffice imo.
SeanFeldman
changed the title
|
Marked PR as |
|
@0xced could you please
|
|
@nemakam would you like to review as well? |
|
@0xced @SeanFeldman - I'm somehow not convinced we need this feature. We are complicating a lot of things and adding new params just so that you could you AutoComplete=true. RegisterMessageHandler() is for extreme novice users who do not know the intricacies of SB and just want a working solution. We do have a way for advanced users to do whatever they wish to. I feel this feature will confuse more people than help (since abandoning with some property seems to be a very minority case). Sorry to be the party popper here. The code itself looks good, but I don't think we would need this feature. |
|
I'm thinking more in terms of number of novice users who are going to struggle understanding what to pass for that property as compared to number of users who will benefit with this feature. |
I'll note that @0xced is not a novice user. As well as message handler while initially planned as an entry level API is actually more sophisticated for "newbies" that Send/Receive pattern. When it comes to implementing a message pump, suspect at that point we no longer talking about novice users at all. It's either using message handler to avoid the headaches or creating one from a scratch if the built-in doesn't cut it.
The API is extended, meaning users will have both options - w/ or w/o properties to update on abandon operation. Yes it adds to the public API surface, but it also adds flexibility to the MessageHandler API, which will auto-abandon but not allow to update properties. Thinking about it even more, if you want to update properties today, you have to entirely opt-out of MessageHandler, which is a shame if you don't want to build your own message pump. So when it comes to 👍/👎 I tend to think that this API extension is a nice addition. |
You should just be able to do that within the handler right. The feature that you will miss out on is AutoComplete. If you can do a try-catch within your code, and abandon the way you want to abandon, that should do it for you. |
|
That’s currently what I’m doing (try+complete/catch+abandon in the handler) and precisely what led me to open this pull request. But I agree that it complicates the API. I have thought about a simpler solution that I just proposed in #671. |
|
Closing in favour of #671 |
This is useful to store exception details when abandoning a message and still keeping the convenience of registering a message or session handler with AutoComplete = true