[Service Bus] Move settlement & renewLock methods from message to receiver#11962
Conversation
ramya-rao-a
requested review from
HarshaNalluru,
bterlson,
chradek,
richardpark-msft and
xirzec
as code owners
ghost
added
the
ramya-rao-a
changed the title
|
Successful live test run: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=583761&view=results |
| createReceiver(queueName: string, options: CreateReceiverOptions<"receiveAndDelete">): ServiceBusReceiver<ServiceBusReceivedMessage>; | ||
| createReceiver(topicName: string, subscriptionName: string, options?: CreateReceiverOptions<"peekLock">): ServiceBusReceiver<ServiceBusReceivedMessageWithLock>; | ||
| createReceiver(topicName: string, subscriptionName: string, options: CreateReceiverOptions<"receiveAndDelete">): ServiceBusReceiver<ServiceBusReceivedMessage>; | ||
| createReceiver(queueName: string, options?: CreateReceiverOptions<"peekLock">): ServiceBusReceiver; |
There was a problem hiding this comment.
If we aren't returning different types, there is no point in using the generics.
It could just be CreateReceiverOptions with an attribute "receiveMode".
There was a problem hiding this comment.
same for acceptSession and any other generics involving receiveMode
There was a problem hiding this comment.
Please see the note in the PR description on this topic
I have retained the overloads for createReceiver and createSessionReceiver methods even though there is no difference between the overloads anymore. This is to facilitate the next step of ensuring that users in ReceiveAndDelete mode do not see the settlement methods or the renewMessageLock method.
richardpark-msft
left a comment
richardpark-msft
left a comment
There was a problem hiding this comment.
It all looks pretty much how you'd expect, thanks for doing this.
What happened with backupMessageSettlement.spec.ts though?
The tests here closed the user facing receiver and then tried to settle messages. This is no longer a flow we would be expecting people to use. Rather, the tests need to close the underlying AMQP receiver link. I am doing this in 52b585f. The session related tests are failing which needs to be followed up. I dont mind removing them altogether as we dont have back up message settlement plan for sessions |
| await msgBatch[0].complete(); | ||
| await receiver.completeMessage(msgBatch[0]); | ||
| } else { | ||
| should.equal(errorWasThrown, false, "Error was thrown for sessions without session-id"); |
There was a problem hiding this comment.
I think I meant ...thrown for messages without session-id"
| await beforeEachTest(withSessionTestClientType); | ||
| await testComplete(); | ||
| }); | ||
| // it(withSessionTestClientType + ": complete() removes message", async function(): Promise<void> { |
There was a problem hiding this comment.
Why not uncomment these session tests?
It'll ensure that we are indeed providing a decent error message in the case of sessions IMO.
There was a problem hiding this comment.
As mentioned #11962 (comment), these tests are failing and would need follow up.
3 participants
This PR moves the settlement and renewLock methods from the message to the receiver as per #11926 with minimal changes to the code. Things to clean up in a following PR post GA
I have retained the overloads for
createReceiverandcreateSessionReceivermethods even though there is no difference between the overloads anymore. This is to facilitate the next step of ensuring that users in ReceiveAndDelete mode do not see the settlement methods or the renewMessageLock method. There are two approaches to this which are experimented with in