Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: improve retry behavior with resources at MG scope #681

Merged
merged 7 commits into from
Dec 5, 2024

Conversation

matt-FFFFFF
Copy link
Member

@matt-FFFFFF matt-FFFFFF commented Nov 26, 2024

As per internal thread:

  • Improved 403 handling for MG resources to include child resources
  • Fix bug in get after put retry timeout so the environment variable works properly
  • Added debug output to retry

I could also pass the regex to the get after put retry, but since the body is nil then the retry func catches the issue where we get a 403. WDYT @ms-henglu

@ms-henglu
Copy link
Member

ms-henglu commented Nov 27, 2024

Hi @matt-FFFFFF ,

Thank you for the PR, LGTM.

I could also pass the regex to the get after put retry, but since the body is nil then the retry func catches the issue where we get a 403. WDYT

I think it's better to allow the user specified retry block to control retry policy of the GET after PUT. Maybe combine both 404 or empty body retry with the user configured retry policy? By doing that, user could also specify the max elapsed time in the configuration and get rid of the environment variables.

@matt-FFFFFF
Copy link
Member Author

matt-FFFFFF commented Nov 30, 2024

Hi @ms-henglu I have rebased and updated this PR to pass the regex messages to the getafterput client.

The max time was omitted from the retry object because we rely on the resource timeout value (context deadline). I think having two timeouts could be confusing?

WDYT?

Copy link
Member

@ms-henglu ms-henglu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, LGTM.

@ms-henglu ms-henglu requested a review from magodo December 2, 2024 06:11
@ms-henglu ms-henglu added this to the v2.2.0 milestone Dec 5, 2024
@ms-henglu ms-henglu merged commit ac4a733 into Azure:main Dec 5, 2024
12 checks passed
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.

2 participants