Skip to content
This repository has been archived by the owner. It is now read-only.

Do not retry when there's an ambient transaction #621

Merged
merged 4 commits into from Dec 14, 2018

Conversation

Projects
None yet
2 participants
@SeanFeldman
Copy link
Collaborator

commented Dec 13, 2018

Fixes #615

Fix is following recommendation made on the issue:

The RetryPolicy should understand whether the operation is within a transaction scope or not. If it is, it should never retry. And this should be documented properly.

TODO

  • Implement change to skip retries when an ambient transaction is detected
  • Document this behavior (XMLdocs, anywhere else?)
  • Verify with tests
  • Release as a hotfix 3.2.1

@SeanFeldman SeanFeldman requested a review from Azure/azure-service-bus-write as a code owner Dec 13, 2018

@SeanFeldman SeanFeldman added this to the 3.2.1 milestone Dec 13, 2018

@SeanFeldman SeanFeldman self-assigned this Dec 13, 2018

@SeanFeldman SeanFeldman changed the title Do not retry when there's an ambient transaction [WIP] Do not retry when there's an ambient transaction Dec 13, 2018

@SeanFeldman

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 13, 2018

ReceiveAndDelete tests are flaky imo.

E.g. QueueClientTests.ReceiveDeleteTest is failing. It's supposed to send 10 messages and receive back 10. The way it's implemented is using TestUtility.ReceiveMessagesAsync(count). Internally, this method is running a tight loop of 5 attempts to receive all messages from an entity. Problem with this approach is that tight loop might be too fast for the broker to return all messages, resulting in a flaky tests, failing due to incorrect number of messages returned. For the test verification, I'll add a delay between attempts to see if that helps.

Modify TestUtility.ReceiveMessagesAsync in a backwards compatible way…
… to allow timeout specification to handle flaky tests

@SeanFeldman SeanFeldman changed the title [WIP] Do not retry when there's an ambient transaction Do not retry when there's an ambient transaction Dec 13, 2018

@SeanFeldman

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 13, 2018

Green and ready.

@nemakam nemakam merged commit a1e637d into Azure:dev Dec 14, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details

@SeanFeldman SeanFeldman deleted the SeanFeldman:retry-policy-issue-615 branch Dec 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.