standard-ai / ya-gcp

Apache License 2.0
7 stars 8 forks source link

Make emulator connection retries configurable #32

Closed matthew-healy closed 1 year ago

matthew-healy commented 1 year ago

We're seeing test flakiness when using the Bigtable and Pub/Sub emulators in CI. As far as we can tell this is caused by the connection timing out. I've tested this change in our CI (by bumping the timeout to 30 seconds) and it fixes the issue.

There are almost certainly better ways to pass this config in, so please feel free to suggest improvements.

jneem commented 1 year ago

Sure, it makes sense to me that this be configurable. At some point it probably makes sense to switch to a builder-style configuration, like Emulator::new().project("hello").connection_limit(100).build(), but maybe there aren't yet enough configuration parameters to justify it...

rnarubin commented 1 year ago

+1 on a builder. Those constructor names get unwieldy over 2 params IMO

matthew-healy commented 1 year ago

Yep, makes sense. I'll try to find time to make that change today. Thanks folks.

matthew-healy commented 1 year ago

I updated the code to use builders, and I agree that it's much nicer.

I did it in two different ways, split across two different commits. The first time I essentially just copied the way the existing constructors worked, which you can find in 372ca8c.

While doing this, I noticed that the default values for the project & instance only really made sense for the test suite in this repository. It seemed a little odd to have Emulator::new().await return something that was actually very unlikely to be what consumers wanted, so I removed the defaults and then used typestates to force consumers to provide a project (and, in the case of the bigtable emulator, an instance). I think this works pretty nicely, but I deliberately did it in its own commit so that I can drop it in case not everyone is as enamoured by typestates as I am. :grin:

matthew-healy commented 1 year ago

This fails on 1.63 as into_future wasn't stablised until 1.64. Any objections to updating the version on CI?