[Service Bus] Update to use core-amqp #5324
Conversation
…to sb-track-2-p2
ramya0820
requested review from
chradek,
KarishmaGhiya,
mikeharder,
ramya-rao-a and
ShivangiReja
as code owners
There was a problem hiding this comment.
One of the big changes in core-amqp was that the retry logic for $management operations (in managementClient.ts file) was moved out of the RequestResponseLink class.
By not including those changes in this PR, we are effectively removing all retries for $management operations. We cannot have the feature branch in this state of no retries
| @@ -65,7 +65,8 @@ | |||
| }, | |||
| "sideEffects": false, | |||
| "dependencies": { | |||
| "@azure/core-amqp": "1.0.0-preview.1", | |||
| "@azure/core-amqp": "1.0.0-preview.3", | |||
| "@azure/core-http": "1.0.0-preview.3", | |||
There was a problem hiding this comment.
Why the dependency on core-http?
There was a problem hiding this comment.
Oops, might have been a merge error - fixed
| @@ -106,6 +107,8 @@ export class Receiver { | |||
| renewMessageLock(lockTokenOrMessage: string | ServiceBusMessage): Promise<Date>; | |||
| } | |||
|
|
|||
| export { RetryOptions } | |||
There was a problem hiding this comment.
I see RetryOptions being exported from the Service Bus library here, but it is not clear which of the APIs is now allowing the use of RetryOptions by the user
There was a problem hiding this comment.
This hsould be clear once #4376 is picked up
| connectionHost: this._context.namespace.config.host, | ||
| delayInSeconds: 15 | ||
| retryOptions: { | ||
| maxRetries: Constants.defaultMaxRetries, |
There was a problem hiding this comment.
shouldn't this be Constants.defaultMaxRetriesForConnection?
| times: Constants.defaultConnectionRetryAttempts, | ||
| connectionHost: this._context.namespace.config.host, | ||
| delayInSeconds: 15 | ||
| retryOptions: { maxRetries: Constants.defaultMaxRetries, retryDelayInMs: 15000 }, |
There was a problem hiding this comment.
Shouldn't this be Constants.defaultMaxRetriesForConnection?
Yes, I'm aware of this, since this is retrofitting core-amqp - looking to scope down to it and add retries in a different PR, and could be combined with #4376 since that's about updating the retries.
Is there a reason why? |
|
While moving this work to #4376 decreases the scope in this PR, it increases the scope of #4376. My hope was to maintain status quo as far as the feature set is concerned while we make the move to use core-amqp, therefore, didnt want the branch to lose the retry features. But, its fine if we want to use the approach of adding retries later. |
* Pin core-amqp version to preview.1 * Update lock file * Fix merge conflicts * [Service Bus] Update to use core-amqp (#5324) * Use core-amqp in atom management client, comment tests that use token provider * [Service Bus] Update to use latest core-amqp (#6861) * [Service Bus] Update constructors to add overloads and use Azure Identity (#5436) * sync-versions: rhea-promise and @azure/core-amqp for service-bus * resolve merge conflicts appropriately * API Report File for "@Azure/service-bus" * Address feedback * SERVICEBUS_CONNECTION_STRING_BROWSER remove from karma.conf * update pnpm-lock file * update API Report File for "@Azure/service-bus" * remove rhea and rhea-promise dependencies * add "rhea-promise" back Co-authored-by: Ramya Rao <[email protected]> Co-authored-by: ramya0820 <[email protected]>
Makes Service Bus build fine with latest Track 2 core dependencies.
This would be precursor step to start making changes for Service Bus Track 2.