Share connection across different clients #881
Conversation
cretz
requested review from
mfateev,
Sushisource,
alexshtin,
Spikhalskiy,
bergundy and
mmcshane
as code owners
…ction # Conflicts: # test/integration_test.go
| // this package and cannot be wrapped. Currently, this always attempts an eager | ||
| // connection even if the existing client was created with NewLazyClient and has | ||
| // not made any calls yet. | ||
| func NewClientFromExisting(existingClient Client, options Options) (Client, error) { |
There was a problem hiding this comment.
It might make sense to make a narrower options type here with just the stuff that won't be ignored.
If that can be re-used in a backwards compat way inside the existing options, that's gravy. If not it might still be worth it just to make this less potentially confusing, but up to you.
There was a problem hiding this comment.
In theory I like splitting the struct into "connection options" and "other options" but I don't think it's worth it for this feature that most people won't ever use
There was a problem hiding this comment.
It might make sense to make a narrower options type here with just the stuff that won't be ignored.
This can't be done in a backwards compatible way. I'd have to make a whole new struct. This would entail just copying all but two fields of the existing struct just for this method and hoping I properly keep them and their docs in sync when adding more. I think it's reasonable to reuse these options explaining which are unused.
| // this package and cannot be wrapped. Currently, this always attempts an eager | ||
| // connection even if the existing client was created with NewLazyClient and has | ||
| // not made any calls yet. | ||
| func NewClientFromExisting(existingClient Client, options Options) (Client, error) { |
There was a problem hiding this comment.
In theory I like splitting the struct into "connection options" and "other options" but I don't think it's worth it for this feature that most people won't ever use
| // Since this interceptor can be used for clients of different name, we | ||
| // attempt to extract the namespace out of the request. All namespace-based | ||
| // requests have been confirmed to have a top-level namespace field. |
There was a problem hiding this comment.
Does this deserve a comment in the proto file? I mean I wouldn't do a PR just for that, but bundle it into your next api PR? :)
There was a problem hiding this comment.
Yeah, if I remember on my next one I'll add something like "Some clients expect all namespace-based RPC calls to have a single namespace string field in the request".
What was changed
NewClientFromExistingfunction to create a new client with the same connection but different client optionsWhy?
Users want to use the same connection for multiple namespaces
Checklist