🌱 single connection for grpc options. #97
Conversation
Signed-off-by: morvencao <[email protected]>
|
/assign @qiujian16 @skeeey |
| @@ -43,6 +43,10 @@ type KeepAliveOptions struct { | |||
|
|
|||
| // Dial connects to the gRPC server and returns a gRPC client connection. | |||
| func (d *GRPCDialer) Dial() (*grpc.ClientConn, error) { | |||
| // Return the cached connection if it exists. | |||
There was a problem hiding this comment.
should we add a lock here? and what happens when it is not in ready state?
There was a problem hiding this comment.
grpc.ClientConn is thread safe from the doc: https://github.com/grpc/grpc-go/blob/master/Documentation/concurrency.md#clients
If we return a connection which is not ready, the base client will not get a new connection in reconnect cases, see:
https://github.com/open-cluster-management-io/sdk-go/blob/main/pkg/cloudevents/generic/baseclient.go#L63
There was a problem hiding this comment.
please add some comments here.
There was a problem hiding this comment.
what happens if the two goroutine are dialing at the same time?
There was a problem hiding this comment.
I misunderstood, the GRPCDialer needs a lock to protect the conn since multiple callers (cloudevents clients) may access it concurrently. Updated the code and comments.
Signed-off-by: morvencao <[email protected]>
morvencao
force-pushed
the
br_grpc_conn
branch
3 times, most recently
from
367c920 to
0c3e76f
Compare
| // Return the cached connection if it exists and is ready. | ||
| // Should not return a nil or unready connection, otherwise the caller (cloudevents client) | ||
| // will not use the connection for sending and receiving events in reconnect scenarios. | ||
| d.mu.RLock() |
There was a problem hiding this comment.
I think we only lock once at the beginning to avoid building the connection multiple times among the goroutines
|
LGTM |
|
could add an integration test that starts multiple thread and dial at the same time? |
|
@qiujian16 added an integration test:
|
Signed-off-by: morvencao <[email protected]>
morvencao
changed the title
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: morvencao, qiujian16 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:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit b3879d7
into
open-cluster-management-io:main
Summary
This PR ensures one single connection for each grpc/mqtt option.
Related issue(s)
Fixes #