search

GoogleCloudPlatform / testgrid

Apache License 2.0
193 stars 68 forks source link

Add retry when snapshotting configuration. #1268

Closed michelle192837 closed 8 months ago

michelle192837 commented 9 months ago

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 commented 9 months ago

/retest

michelle192837 commented 8 months ago

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

michelle192837 commented 8 months ago

Test should be fixed, ready for review!

michelle192837 commented 8 months ago

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).

google-oss-prow[bot] commented 8 months ago

[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](https://github.com/GoogleCloudPlatform/testgrid/blob/main/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 commented 8 months ago

/hold cancel