Skip to content
This repository has been archived by the owner on Oct 12, 2023. It is now read-only.

Only retry with retryable amqp errors for sender #201

Merged
merged 1 commit into from
Jan 15, 2021

Conversation

gavinfish
Copy link
Contributor

@gavinfish gavinfish commented Dec 21, 2020

Fix #200

  1. When sender.trySend() fails with amqp error, it is not necessary to retry for all the conditions. Only retry with the below errors:
  • amqp.Error
    • com.microsoft:server-busy
    • com.microsoft:timeout
    • com.microsoft:operation-cancelled
    • com.microsoft:container-close
  • amqp.DetachError

You can refer to python sdk with similar logic at
https://github.com/Azure/azure-sdk-for-python/blob/c9240aa68c2870a1a72249b4c86f3ca95551cc29/sdk/servicebus/azure-servicebus/azure/servicebus/exceptions.py#L58-L80

  1. Reduce the retry times from 10 to 3, delay from 10s to 1s. Take com.microsoft:server-busy with 10s delay which is suggested from service bus error.

"*Error{Condition: com.microsoft:server-busy, Description: The request was terminated because the namespace azuresbtests-fqfclayc is being throttled. Error code : 50009. Please wait 10 seconds and try again. Reference:3f49ca31-e885-4491-853d-ffaa8293c013, TrackingId:1a2c36240000000a0024729e5d39641b_G17_B2, SystemTracker:azuresbtests-fqfclayc:Queue:max-send-codvkdzlfb, Timestamp:2019-07-25T08:11:16, Info: map[]}

  1. Do not close the sender when the error cannot be recovered. It is not necessary especially that the lib cannot handle it well with Topic struct well now. Better to add an option to determine whether close the sender in the future.

errors.go Outdated Show resolved Hide resolved
errors.go Outdated Show resolved Hide resolved
sender.go Outdated Show resolved Hide resolved
Copy link
Member

@catalinaperalta catalinaperalta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@catalinaperalta catalinaperalta dismissed their stale review December 22, 2020 22:58

Need to update changelog and add method comment.

sender.go Outdated Show resolved Hide resolved
@catalinaperalta
Copy link
Member

Could you also add a test to mock receiving these errors and also update the changelog? I'll then approve again.

@gavinfish gavinfish force-pushed the sender-retry branch 2 times, most recently from 57eca53 to 9a836ff Compare December 24, 2020 15:14
@gavinfish
Copy link
Contributor Author

Could you also add a test to mock receiving these errors and also update the changelog? I'll then approve again.

@catalinaperalta I have some problems to add unit tests here. As amqp.Sender is a struct without interface, I cannot easily mock it to return different error messages here. I can also abstract functions to wrap calling amqp.Sender and mock the wrapper, but this is a little bit wired just for test.

sender.go Outdated Show resolved Hide resolved
@catalinaperalta catalinaperalta merged commit 78c960d into Azure:master Jan 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sender returns error "the connection has been closed: Sender" after "context deadline exceeded"
3 participants