search

liftbridge-io / go-liftbridge

Go client for Liftbridge. https://github.com/liftbridge-io/liftbridge
Apache License 2.0
68 stars 18 forks source link

Add a connection timeout #98

Closed Jmgr closed 3 years ago

Jmgr commented 3 years ago

The current API does not provide a way to cancel a connection while it is in progress. If the server is not reachable the default GRPC timeout applies, which is a few minutes. It could be good for liftbridge.Connect to accept a connection timeout.

This could be implemented by using grpc.DialContext instead of grpc.Dial.

Jmgr commented 3 years ago

Having given this more thought, I see multiple options on how to present this to the end user.

  1. Pass a context to the Connect function that can be used to cancel the connection and set a timeout to it. This I believe would be the standard Go way, but it would mean changing the API so probably not applied until v3.
  2. Pass a timeout value as an option. This is a bit tricky to implement because we need to keep a context alive while the connection is established. We could set the gRPC connection as blocking, but that would change the behaviour of the Connect function.
  3. Pass a timeout value as an option, as well as a context used only during the connection. That would solve the above implementation issue, but I'm not sure if that would be very intuitive to use unless the gRPC connection is set to be blocking.

Also there are two types of connections, the one initiated by the user by calling Connect, but also other ones directed to the brokers. Do we want to set the same timeout to both, or separately?

tylertreat commented 3 years ago

I like passing a Context. What about adding a new function, ConnectCtx that accepts a Context to avoid a breaking API change. Then, in v3, we can replace Connect with ConnectCtx?

Also there are two types of connections, the one initiated by the user by calling Connect, but also other ones directed to the brokers. Do we want to set the same timeout to both, or separately?

It would probably only make sense to re-use the timeout if it was passed in a Connect option rather than a Context? If we pass a Context to Connect, do we need a separate option for configuring these "background" connections?

Jmgr commented 3 years ago

I like passing a Context. What about adding a new function, ConnectCtx that accepts a Context to avoid a breaking API change. Then, in v3, we can replace Connect with ConnectCtx?

Sounds good :+1:

Also there are two types of connections, the one initiated by the user by calling Connect, but also other ones directed to the brokers. Do we want to set the same timeout to both, or separately?

It would probably only make sense to re-use the timeout if it was passed in a Connect option rather than a Context? If we pass a Context to Connect, do we need a separate option for configuring these "background" connections?

Good point, probably not needed then.