Support reading from dead-letter queues with transport seam

[ramonsmits]

Support reading from dead-letter queues with transport seam:

Create ReceiveSettings with property key Azure.Messaging.ServiceBus.SubQueue and the value to match DeadLetter:

var queueProperties = new Dictionary<string, string>(1);
queueProperties[typeof(SubQueue).FullName] = SubQueue.DeadLetter.ToString();

var receiveSettings = new ReceiveSettings(
    id: inputQueue,
    receiveAddress: new QueueAddress(inputQueue, properties: queueProperties),
    usePublishSubscribe: false,
    purgeOnStartup: false,
    errorQueue: errorQueue

[SzymonPobiega]

Nitpicking: the changes to other unrelated files make it hard to figure out the essence.

[ramonsmits]

Nitpicking: the changes to other unrelated files make it hard to figure out the essence.

@SzymonPobiega thnx! That was accidentally applied due to Rider formatting being different from VS so I reverted these.

[danielmarbach]

Honestly, I think this is too much of a blunt hammer for what you want to achieve, especially considering overriding the processor options allows overriding the receive mode which then can create misaligned settings with the transaction mode. One could flip the auto-completion, concurrency can be tweaked and prefetch settings calculation is then also potentially misaligned.

I think this is potentially too dangerous to expose, given the intricacies of the pump logic.

Also has someone verified sends with atomic receive would work with sub queue processing?

[ramonsmits]

Honestly, I think this is too much of a blunt hammer for what you want to achieve, especially considering overriding the processor options allows overriding the receive mode which then can create misaligned settings with the transaction mode. One could flip the auto-completion, concurrency can be tweaked and prefetch settings calculation is then also potentially misaligned.

I just squashed all commits but otherwise you could have seen that before I introduced a new class AzureServiceBusReceiverSettings that had a DLQ property. It had the intent to purely support ingesting dead-letter queues. It was refactored into this to just allow any customization if needed. IMHO most users will not be touching this API unless they really need something specific.

Yes, they can do all that but that is the intent of this API.

I think this is potentially too dangerous to expose, given the intricacies of the pump logic.

It was brought up to make this/these APIs internal but that means that we need to list our own dependencies in this transport unless we go for the reflection hack.

IMHO this is an API for power users. An alternative is exposing this in a different way.

I do think the XML doc can be improved to express that it can be very dangerous just like you did on the other PR.

Also has someone verified sends with atomic receive would work with sub queue processing?

No, but I think ANY customization's on our integration APi's should be considered unsupported. Unsupported as in, we can't guarantee correct behavior when someone has the power to make harmful changes.

[danielmarbach]

By tweaking this any safety nets and assumptions in the pump code can go out of the window. This is not a feature. It is a foot gun. I'm happy to help find other ways and look at the use cases to brainstorm different approaches.

[ramonsmits]

By tweaking this any safety nets and assumptions in the pump code can go out of the window. This is not a feature. It is a foot gun. I'm happy to help find other ways and look at the use cases to brainstorm different approaches.

As mentioned, we already went from a specific to a generic approach. Specifically because of advanced native integration requirements. In this case we can use the API ourselves. If you think this is really to powerful which I don't share we can just use the internal hack on it. If you have a really better suggestion to enable DLQ support I'm all ears but keep in mind the TF pretty much already agreed to go for the generic approach.

[danielmarbach]

Didn't we agree to first check with the team whether this is officially supported? I'm still in the process of talking to them

[ramonsmits]

Didn't we agree to first check with the team whether this is officially supported? I'm still in the process of talking to them

@danielmarbach we have a test, it works. IMHO not a blocker to await the response. If the team responds with feedback that that an unsupported configuration we'll add an patch that validates the transaction mode.

[danielmarbach]

Didn't we agree to first check with the team whether this is officially supported? I'm still in the process of talking to them

@danielmarbach we have a test, it works. IMHO not a blocker to await the response. If the team responds with feedback that that an unsupported configuration we'll add an patch that validates the transaction mode.

That's totally fine. I asked because we were discussing something else and there was no comment that indicated a change of direction.

In the meantime, I got confirmation this is an officially supported scenario. Will add some documentation internally