[Service Bus] Support cancellation on $management operations #8159
Conversation
ramya-rao-a
requested review from
bterlson,
chradek,
HarshaNalluru and
richardpark-msft
as code owners
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -251,6 +257,13 @@ export { TokenCredential } | |||
|
|
|||
| export { TokenType } | |||
|
|
|||
| // @public | |||
| export interface TracingOptions { | |||
There was a problem hiding this comment.
I don't think I've ever noticed how funny this looks.
Also, now it makes me wonder why SpanOptions doesn't contain a field called spanOptions. :)
There was a problem hiding this comment.
Yeah, personally I think I prefer just putting the tracingOptions? field on whatever currently extends TracingOptions, since this only has one field and is now exposed to users when it probably wouldn't be used otherwise. But that's just my opinion 😄
There was a problem hiding this comment.
Yeah, @ramya-rao-a 's made this argument in the past and I keep going back and forth on it. I mostly just worry about accidentally making it inconsistent but we can probably counter that by simply having a compile-test like you did recently for core-amqp (I believe)?
There was a problem hiding this comment.
Any change we want to do here is something I want to ensure is the same between Event Hubs and Service Bus. We already have AMQP and HTTP packages diverging a little, I would prefer to have as much common coding and design patterns between Service Bus and Event Hubs for our own sanity when we move between these 2 projects when in maintainence mode
| `[${this._context.namespace.connectionId}] The send operation on the Sender "${this.name}" with ` + | ||
| `address "${this.address}" has been cancelled by the user.`; | ||
| // Cancellation is user-intended, so log to info instead of warning. | ||
| log.error(desc); |
There was a problem hiding this comment.
The comment doesn't seem to match what you're doing? Or is it that 'log' is automatically info or something?
There was a problem hiding this comment.
Yup, copied from Event Hubs. There, we have moved on to use the new logger that lets you use log levels. We dont have that in Service Bus yet
| const removeListeners = (): void => { | ||
| clearTimeout(waitTimer); | ||
| if (abortSignal) { | ||
| abortSignal.removeEventListener("abort", onAborted); |
There was a problem hiding this comment.
abortSignal?.removeEventListener() and remove the if()?
There was a problem hiding this comment.
I was trying to keep the same code patterns as Event Hubs so that a quick glance at either would tell us that we are dealing with the same thing. If we make the decision to use option chaining, then I'd prefer to do it in both places at once.
| }; | ||
|
|
||
| if (abortSignal) { | ||
| abortSignal.addEventListener("abort", onAborted); |
There was a problem hiding this comment.
abortSignal?.addEventListener() and remove the if()?
There was a problem hiding this comment.
I was trying to keep the same code patterns as Event Hubs so that a quick glance at either would tell us that we are dealing with the same thing. If we make the decision to use option chaining, then I'd prefer to do it in both places at once.
| @@ -267,10 +308,12 @@ export class MessageSender extends LinkEntity { | |||
| }; | |||
| return reject(translate(e)); | |||
| }; | |||
| const waitTimer = setTimeout(actionAfterTimeout, retryTimeoutInMs); | |||
There was a problem hiding this comment.
Not sure this is a good change - prior to this you only set the timeout if it wasn't open.
Is there a reason this was needed?
There was a problem hiding this comment.
I was trying to play around placing this such that TS stops complaining about waitTimer. Needs more thought. Also, temp commit
| } = {} | ||
| ): Promise<number> { | ||
| const retryOptions = options.retryOptions || {}; | ||
| async getMaxMessageSize(): Promise<number> { |
There was a problem hiding this comment.
What is the criteria for when a method takes it's own OperationOptions vs only inheriting it from the service client?
There was a problem hiding this comment.
OperationOptions are never inherited from the client. They are specific to each operation/method.
Here, getMaxMessageSize() is not a method user would call. Its called from createBatch() which in turn is what the user would call. So, createBatch() gets to have OperationOptions.
I didnt pass it to getMaxMessageSize() because all this function does is initialize the link and we dont yet have the way to cancel an initialization that is in progress. See #4422
|
And tests? |
|
@richardpark-msft The changes to the messageSender.ts file got into this PR by accident which I have now reverted. Marking all your comments on this file as resolved as I wanted to restrict this PR only to $management operations The commit message was also "temp" :) cf46a27 |
|
Tests have been added with ab06ab1 |
| @@ -240,17 +246,24 @@ export interface SubscribeOptions extends OperationOptions, MessageHandlerOption | |||
|
|
|||
| // @public | |||
| export interface SubscriptionRuleManager { | |||
| addRule(ruleName: string, filter: boolean | string | CorrelationFilter, sqlRuleActionExpression?: string): Promise<void>; | |||
| addRule(ruleName: string, filter: boolean | string | CorrelationFilter, sqlRuleActionExpression?: string, options?: OperationOptions): Promise<void>; | |||
There was a problem hiding this comment.
This does result in multiple optional arguments. My preference here would be to have overloads rather than moving the action expression into the options bag. I want to take that call outside the scope of this PR
There was a problem hiding this comment.
This being both a preview and a probably not-used-that-much API makes it a good candidate for that.
| ); | ||
| const result = await this._makeManagementRequest(request, { | ||
| abortSignal: options?.abortSignal, | ||
| requestName: "renewLock" |
There was a problem hiding this comment.
[NIT]
Why pass the "request_name" here as opposed to passing options directly for the rest of the methods?
Should we be consistent and define the request names in this file for all the methods?
There was a problem hiding this comment.
I didnt want to touch renewLock and updateDispositionStatus in this PR as there are still some decisions that we want to take there regarding cancelling and tracing options.
|
Thanks all! |
This PR updates all the $management operations on the sender, receiver, session receiver and the rule manager to respect the abort signal. Exceptions are
The
RequestResponse.sendRequest()already respects the abort signal. So, the majority of the work in this PR is to ensure that the option is plumbed through.Send operations will be updated in a separate PR