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

Add retry when snapshotting configuration. #1268

Merged
merged 1 commit into from
Dec 1, 2023

Conversation

michelle192837
Copy link
Collaborator

@michelle192837 michelle192837 commented Nov 28, 2023

Adding a quick retry to mitigate issues where the configuration is (temporarily) not available on startup.

Added a unit test for this as well (probably could be cleaner, but it should verify that the retry actually works).

ETA: The actual change to add a retry is in config_snapshot.go. Everything else is an update to add a unit test for the retry, and enhance the storage fake to allow that (with a lock to prevent data races on map reads/writes for the fake Opener).

@michelle192837
Copy link
Collaborator Author

/retest

@michelle192837
Copy link
Collaborator Author

Ah, there's a legitimate race in the new fake logic I believe. Taking a look at that.

@michelle192837
Copy link
Collaborator Author

Test should be fixed, ready for review!

Copy link
Collaborator

@chases2 chases2 left a comment

Choose a reason for hiding this comment

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

The code itself looks good (after digging around a bit in this new "retry" library) and the tests demonstrates the API well (ie: you will get a config OR you will get an error, etc), but I think it could be cleaner. Please let me know what you think!

config/snapshot/config_snapshot_test.go Show resolved Hide resolved
config/snapshot/config_snapshot_test.go Show resolved Hide resolved
config/snapshot/config_snapshot_test.go Outdated Show resolved Hide resolved
@michelle192837
Copy link
Collaborator Author

The code itself looks good (after digging around a bit in this new "retry" library) and the tests demonstrates the API well (ie: you will get a config OR you will get an error, etc), but I think it could be cleaner. Please let me know what you think!

Thanks for catching these! Agree on the suggestions, implemented them and updated the logic in the fake to be clearer (pretty sure I hit some issues on the boolean conditions).

Copy link
Collaborator

@chases2 chases2 left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for taking the extra time on the tests. :D

Feel free to merge when ready; I only see one tiny nit after the refactor.

/hold

config/snapshot/config_snapshot_test.go Show resolved Hide resolved
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chases2, michelle192837

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [chases2,michelle192837]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@michelle192837
Copy link
Collaborator Author

/hold cancel

@google-oss-prow google-oss-prow bot merged commit 5ec0994 into GoogleCloudPlatform:main Dec 1, 2023
5 checks passed
@michelle192837 michelle192837 deleted the retry branch December 1, 2023 22:30
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.

2 participants