This diff introduces a utility function to create a thread that tracks the state of a running gRPC server. This utility function will eventually be used by the DagsterGraphqlContext in order to appropriately reload handles and inform the Dagit front-end client of updates.
unit tests to check that the correct callback functions are called in all the possible server lifecycles
nice - thanks for the thorough testing. the only question I have is if we can get away with not introducing a new API just for the test (see inline)? Maybe there's a sublety there that I'm not seeing
i'm confused about the expectation here in the on_error case - if it can't connect what new thread would it be creating?
instead of time.sleep use
shutdown_event.wait(watch_interval)? Then it wakes up right away if the shutdown event is set
(but then you still need to check the event)
same as above
return can be removed?
what's the use case for the fixed server ID? I imagine that will become clear later in the stack?
i'm confused why we need a new API just for this test, vs. terminating the process and starting a new one on the same port (as an actual re-deploy would do)? (with a long enough watch interval / number of retries that it won't timeout while the new server is starting up?)
this was a bit confusing, clarified the comment. the thread shuts down completely in the on_error and on_updated cases. in the next diff, the graphql context handles creating a new handle and subscribing to the events on the new handle
this is to break out of the while True and end the thread
you're totally right, removed this
i'm used to the .wait() going before the .is_set(), might make it more clear what's going on? but meh
oh duh, not sure how i missed that
could add a assert not called after the first interrupt just to verify that it was specifically called in the updated part
could raise an exception here too