xds: make xds client a singleton #4015
Conversation
menghanl
commented
Nov 5, 2020
•
menghanl
commented
- xdsclient.New() no longer takes any input, all configs are from bootstrap file
- added a NewForTesting()
- The returned *Client is a wrapper of the underlying client implementation, for ref-couting
- xds-resolver and xds-server no longer calls bootstrap.NewConfig. It only calls xdsclient.New()
|
This PR doesn't include the change in balancers to call xdsclient.New() (instead of getting client from attributes). Will make that change in a separate PR (because it touches many tests). |
xds/internal/client/client.go
Outdated
| type clientImpl struct { | ||
| done *grpcsync.Event | ||
| config *bootstrap.Config | ||
| cc *grpc.ClientConn // Connection to the xDS server |
There was a problem hiding this comment.
Nit: can we call it the management server instead. Since we have a server-side implementation of xDS now, it is confusing when we say xDS server. Would be better to be unambiguous going forward:
- xDS-enabled gRPC server: to refer to our xDS implementation on the server side
- management server or control plane: to refer to the server implementing the xDS protocol
There was a problem hiding this comment.
Done.
We have "xDS server" all over the tests. Didn't update all of them.
I think in writing, it's not that bad, we probably will write xds.Server to refer to the gRPC server.
| // various dynamic resources. | ||
| // | ||
| // The xds client is a singleton. It will be shared by the xds resolver and | ||
| // balancer implementations, cross multiple ClientConns and Servers. |
| } | ||
| } | ||
|
|
||
| // NewWithConfigForTesting is exported for testing only. For prod uses, call |
There was a problem hiding this comment.
Maybe we can skip the For prod uses, call New(). part?
xds/internal/client/client_test.go
Outdated
| watchExpiryTimeout := time.Duration(0) | ||
| if overrideWatchExpiryTImeout { | ||
| func clientOpts(balancerName string, overrideWatchExpiryTimeout bool) (*bootstrap.Config, time.Duration) { | ||
| watchExpiryTimeout := time.Second * 15 |
There was a problem hiding this comment.
Maybe: watchExpiryTimeout := defaultWatchExpiryTimeout?
| @@ -61,10 +61,10 @@ var gRPCVersion = fmt.Sprintf("%s %s", gRPCUserAgentName, grpc.Version) | |||
| var bootstrapFileReadFunc = ioutil.ReadFile | |||
|
|
|||
| // Config provides the xDS client with several key bits of information that it | |||
| // requires in its interaction with an xDS server. The Config is initialized | |||
| // from the bootstrap file. | |||
| // requires in its interaction with an management server. The Config is | |||
There was a problem hiding this comment.
Nit: s/an management server/the management server/
| Creds: grpc.WithInsecure(), | ||
| NodeProto: &v2corepb.Node{}, | ||
| TransportAPI: version.TransportV2, | ||
| }, 15*time.Second) |
There was a problem hiding this comment.
This test is in a different package (because it needs to import v2).
I added a const for it.
| @@ -16,7 +16,7 @@ | |||
| * | |||
| */ | |||
|
|
|||
| // Package fakeserver provides a fake implementation of an xDS server. | |||
| // Package fakeserver provides a fake implementation of an management server. | |||
- xdsclient.New() no longer takes any input, all configs are from bootstrap file - added a NewForTesting() - The returned *Client is a wrapper of the underlying client implementation, for ref-couting - xds-resolver no longer calls bootstrap.NewConfig. It only calls xdsclient.New()
menghanl
force-pushed
the
xds_client_singleton
branch
from
799ef3e
to
81d73d6
Compare
- xdsclient.New() no longer takes any input, all configs are from bootstrap file - added a NewForTesting() - The returned *Client is a wrapper of the underlying client implementation, for ref-couting - xds-resolver and xds-server no longer calls bootstrap.NewConfig. It only calls xdsclient.New()
