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: remove retry in read as retry for create/update is already handled in respective methods #12028

Merged
merged 4 commits into from
Oct 22, 2024

Conversation

gurusai-voleti
Copy link
Contributor

@gurusai-voleti gurusai-voleti commented Oct 17, 2024

remove retry in read as retry for create/update is already handled in respective methods

Fixes: hashicorp/terraform-provider-google#13464

Release Note Template for Downstream PRs (will be copied)

storage: removed retry on 404s during refresh for `google_storage_bucket`, preventing hanging when refreshing deleted buckets

Copy link

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@rileykarson, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

@github-actions github-actions bot requested a review from rileykarson October 17, 2024 05:32
@gurusai-voleti gurusai-voleti marked this pull request as draft October 17, 2024 05:32
@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Oct 17, 2024
@gurusai-voleti gurusai-voleti marked this pull request as ready for review October 17, 2024 05:34
@gurusai-voleti gurusai-voleti marked this pull request as draft October 17, 2024 05:50
@gurusai-voleti gurusai-voleti marked this pull request as ready for review October 17, 2024 05:50
@gurusai-voleti gurusai-voleti changed the title fix: remove retry in read as retry for create/update I already handled in specific methods fix: remove retry in read as retry for create/update is already handled in specific methods Oct 17, 2024
@gurusai-voleti gurusai-voleti marked this pull request as draft October 17, 2024 07:42
@gurusai-voleti gurusai-voleti marked this pull request as ready for review October 17, 2024 09:21
@gurusai-voleti gurusai-voleti marked this pull request as draft October 17, 2024 09:23
@gurusai-voleti gurusai-voleti changed the title fix: remove retry in read as retry for create/update is already handled in specific methods fix: remove retry in read as retry for create/update is already handled in respective methods Oct 17, 2024
@rileykarson
Copy link
Member

What bug(s) is this addressing / what's the context?

@modular-magician modular-magician added service/storage and removed awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests labels Oct 18, 2024
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 1 file changed, 1 insertion(+), 10 deletions(-))
google-beta provider: Diff ( 1 file changed, 1 insertion(+), 10 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 109
Passed tests: 100
Skipped tests: 9
Affected tests: 0

Click here to see the affected service packages
  • storage

🟢 All tests passed!

View the build log

@gurusai-voleti gurusai-voleti marked this pull request as ready for review October 21, 2024 06:43
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 1 file changed, 1 insertion(+), 10 deletions(-))
google-beta provider: Diff ( 1 file changed, 1 insertion(+), 10 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 109
Passed tests: 100
Skipped tests: 9
Affected tests: 0

Click here to see the affected service packages
  • storage

🟢 All tests passed!

View the build log

Copy link

@rileykarson This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

@kautikdk
Copy link
Member

kautikdk commented Oct 21, 2024

Hi @gurusai-voleti,
As per the changes, we are planning to remove retry logic for read operations. Are we ensured that it is matching storage buckets SLOs? If not can we consider shorter retry time?

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

LGTM, I think this makes sense. We added this to several spots in #5288 and I'm not confident we needed this one + that PR has no extra context on why we would need it here specifically.

Waiting on approval from @kautikdk to merge

@github-actions github-actions bot requested a review from rileykarson October 22, 2024 05:10
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 1 file changed, 1 insertion(+), 10 deletions(-))
google-beta provider: Diff ( 1 file changed, 1 insertion(+), 10 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 109
Passed tests: 100
Skipped tests: 9
Affected tests: 0

Click here to see the affected service packages
  • storage

🟢 All tests passed!

View the build log

Copy link
Member

@kautikdk kautikdk left a comment

Choose a reason for hiding this comment

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

Considering public documentation of cloud storage consistency:https://cloud.google.com/storage/docs/consistency#strongly_consistent_operations, Bucket Listing is strongly consistent operation so we don't need retry logic for bucket read operation.

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

Successfully merging this pull request may close these issues.

State refresh hangs on deleted storage bucket
4 participants