Skip to content

[Service Bus] Timeout for message settlement should result in rejected promise#2886

Merged
ramya-rao-a merged 2 commits into
Azure:masterfrom
ramya-rao-a:msg-settle-timeout
May 14, 2019
Merged

[Service Bus] Timeout for message settlement should result in rejected promise#2886
ramya-rao-a merged 2 commits into
Azure:masterfrom
ramya-rao-a:msg-settle-timeout

Conversation

@ramya-rao-a
Copy link
Copy Markdown
Contributor

When a a request to settle a message is sent, we wait to hear back from the Service that the message was indeed settled i.e we wait for rhea to fire the settled event.
We set a timer for each such request. If the timer runs out before hearing back from the service, we resolve the promise being returned leading the user to believe that the settlement was successful, which may or may not be the cast.

Message send request and any of the operations on the $management link also wait to hear back from the Service. But in case of timeout, they return the ServiceUnavailableError which is retryable and so the sdk retries.

We should try to keep the behavior of timeout when waiting for acknowledgement from the service consistent between the send request, requests not the $management link and the message settlement request.

This requires 2 steps

  • Update the timeout behavior in message settlement to return a rejected promise with the ServiceUnavailableError
  • Make the message settlement request use the retry logic so that it retries on retryable errors.

The first is done in this PR and shipped as part of GA
The second will be tracked in #2885 and will be part of the update after GA

@ramya-rao-a ramya-rao-a requested a review from amarzavery May 14, 2019 17:15
@amarzavery
Copy link
Copy Markdown
Contributor

amarzavery commented May 14, 2019

The 2 jobs are failing due to npm audit issues in @azure/cosmos.
@mikeharder - It will be great if we can run tests only for the files/packages touched in the PR. They should definitely pass. Tests for the entire repo should be run optionally but should not block on merging the PR.

@mikeharder
Copy link
Copy Markdown
Member

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 9 pipeline(s).

@ramya-rao-a
Copy link
Copy Markdown
Contributor Author

Also, ideally this should be an OperationTimeoutError, but we don't use that in the SDK yet and it is not a retryable error.
So for now, to be consistent with the send and management operations we will use ServiceUnavailableError.

Logged #2891 to make OperationTimeoutError retryable

@ramya-rao-a
Copy link
Copy Markdown
Contributor Author

/azp run azure-sdk-for-js - client

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 2 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants