[service-bus] Track 2 - port sender.open() #8636
Conversation
richardpark-msft
requested review from
bterlson,
chradek,
HarshaNalluru,
qiaozha and
ramya-rao-a
as code owners
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -242,15 +242,17 @@ export class ServiceBusClient { | |||
| * Creates a Sender which can be used to send messages, schedule messages to be sent at a later time | |||
| * and cancel such scheduled messages. | |||
| */ | |||
| createSender(queueOrTopicName: string): Sender { | |||
| async createSender(queueOrTopicName: string): Promise<Sender> { | |||
There was a problem hiding this comment.
Should the docs be updated to mention that the sender will be automatically opened when created? This does mean they'd need to explicitly close the sender (or client) if they didn't want it to keep the process open anymore.
There was a problem hiding this comment.
This is interesting. I mean, technically there was no reason for them not to call close() before but, as you're pointing out, they didn't have to.
I can update the doc comment (not sure what I'll write there yet?)
There was a problem hiding this comment.
Maybe it's not super important if we treat this as a new API and that's the behavior (we can mention it in a migration guide for existing users). But it might be worth saying something about the fact that a connection with the service is established if the motivation for this change is that customers want to know if the connection is open or not.
There was a problem hiding this comment.
Yes, adding it to the migration guide is a good idea.
There was a problem hiding this comment.
Migration guide updated and, thanks to you, I remembered to update the samples as well!
| validateEntityNamesMatch(this._connectionContext.config.entityPath, queueOrTopicName, "sender"); | ||
|
|
||
| const clientEntityContext = ClientEntityContext.create( | ||
| queueOrTopicName, | ||
| this._connectionContext, | ||
| `${queueOrTopicName}/${generate_uuid()}` | ||
| ); | ||
| return new SenderImpl(clientEntityContext, this._clientOptions.retryOptions); | ||
| const sender = new SenderImpl(clientEntityContext, this._clientOptions.retryOptions); | ||
| await sender.open(); |
There was a problem hiding this comment.
So with this change, if the sender can't be opened, then the user doesn't get a sender at all and instead get an error right? Is that intentional?
There was a problem hiding this comment.
The original motivation for this was to front-load the work but we had another issue where people wanted a deterministic way to make sure that the connection was actually loaded up and running.
So yes, intentional and you're right, they won't be able to get a sender.
There was a problem hiding this comment.
people wanted a deterministic way to make sure that the connection was actually loaded up and running
Is this meant only for when the sender is 1st created? I ask since it's possible for the connection to be lost at any point after the sender is created.
There was a problem hiding this comment.
This is only meant for when the sender is first created.
However, you brought up a good point that lazy connect would often use retries. So that has now been added :)
…ack2-async-await-receiver
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| validateEntityNamesMatch(this._connectionContext.config.entityPath, queueOrTopicName, "sender"); | ||
|
|
||
| const clientEntityContext = ClientEntityContext.create( | ||
| queueOrTopicName, | ||
| this._connectionContext, | ||
| `${queueOrTopicName}/${generate_uuid()}` | ||
| ); | ||
| return new SenderImpl(clientEntityContext, this._clientOptions.retryOptions); | ||
| const sender = new SenderImpl(clientEntityContext, this._clientOptions.retryOptions); | ||
| await sender.open(); |
There was a problem hiding this comment.
This open() should respect the retry options passed to the client
…Sender(). - Updating migration guide to mention the change to async for createSender() and createSessionReceiver()
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…enderOptions type.
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Havent looked at the test files, but rest of the changes look good.
Please wait for a review from @HarshaNalluru and @chradek before merging
…ore we've had a chance to mock anything. Changing to simulate the connection being closed so open() will get called (and fail properly in our mock)
…testing when createSender() is called.
…re discovered earlier.
…ing the error handling already set up in onDetached.
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| /** | ||
| * The signal which can be used to abort requests. | ||
| */ | ||
| abortSignal?: AbortSignalLike; |
There was a problem hiding this comment.
Just a question that doesn't need to be resolved now, but are we planning to have retry options here as well?
There was a problem hiding this comment.
That should be discussed with the broader team.
Port the important bits from #8329, which added an .open() method to the Sender for track 1.
The main difference here is that ServiceBusClient calls open() on the sender internally, which means we don't need to expose it on the Sender interface.