easwar review 1

xds/internal/clients/config.go

@@ -16,9 +16,8 @@
  *
  */
 
-// Package clients provides implementations of the xDS and LRS clients,
-// enabling applications to communicate with xDS management servers and report
-// load.
+// Package clients provides implementations of the clients to interact with
+// envoy.
 //
 // # xDS Client
 //
@@ -104,13 +103,19 @@ func (sc *ServerConfig) equal(other *ServerConfig) bool {
 	return false
 }
 
-// Authority contains configuration for an xDS control plane [authority].
+// Authority contains configuration for an xDS control plane authority.
 //
-// [authority]: https://www.envoyproxy.io/docs/envoy/latest/xds/core/v3/authority.proto
+// See https://github.com/grpc/grpc/blob/master/doc/grpc_xds_bootstrap_format.md
 type Authority struct {
 	// XDSServers contains the list of server configurations for this authority.
-	// xDS client use the first available server from the list. To ensure high
-	// availability, list the most reliable server first.
+	// The order of the servers in this list reflects the order of preference
+	// of the data returned by those servers. xDS client use the first
+	// available server from the list.
+	//
+	// See [gRFC A71] for more details on fallback behavior when the primary
+	// xDS server is unavailable.
+	//
+	// [gRFC A71]: https://github.com/grpc/proposal/blob/master/A71-xds-fallback.md
 	XDSServers []ServerConfig
 
 	// Extensions can be populated with arbitrary data to be passed to the xDS

xds/internal/clients/lrsclient/client.go

@@ -18,7 +18,7 @@
  *
  */
 
-// Package lrsclient provides an [LRS] (Load Reporting Service) client.
+// Package lrsclient provides an LRS (Load Reporting Service) client.
 //
 // [LRS]: https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/load_stats/v3/lrs.proto
 package lrsclient
@@ -31,7 +31,8 @@ import (
 type LRSClient struct {
 }
 
-// ReportLoad creates a new load reporting stream for the client.
+// ReportLoad creates a new load reporting stream for the client. It creates a
+// LoadStore and return it for the caller to report loads.
 func (c *LRSClient) ReportLoad(serverConfig clients.ServerConfig) *LoadStore {
 	panic("unimplemented")
 }

xds/internal/clients/lrsclient/config.go

@@ -22,9 +22,9 @@ import (
 	"google.golang.org/grpc/xds/internal/clients"
 )
 
-// A Config structure is used to configure an LRS client. After one has been
-// passed to an LRS function it must not be modified. A Config may be used;
-// the LRS package will also not modify it.
+// Config is used to configure an LRS client. After one has been passed to an
+// LRS function, it must not be modified. A Config may be used; the LRS package
+// will also not modify it.
 type Config struct {
 	// Node is the identity of the client application reporting load to the
 	// LRS server.

xds/internal/clients/lrsclient/load_store.go

@@ -23,11 +23,8 @@ package lrsclient
 import "time"
 
 // LoadStore keeps the loads for multiple clusters and services to be reported
-// via LRS. It contains loads to report to one LRS server. It creates
-// multiple stores for multiple servers.
-//
-// A LoadStore is created via LRSClient.ReportLoad and returned for the caller
-// to report loads.
+// via LRS. It contains loads reported to one LRS server. Create multiple
+// stores for multiple servers.
 type LoadStore struct {
 }
 
@@ -43,8 +40,7 @@ func (s *LoadStore) Stats(clusterNames []string) []*Data {
 	panic("unimplemented")
 }
 
-// PerCluster returns the perClusterStore for the given clusterName +
-// serviceName.
+// PerCluster returns the PerClusterReporter for the given cluster and service.
 func (s *LoadStore) PerCluster(clusterName, serviceName string) PerClusterReporter {
 	panic("unimplemented")
 }
@@ -62,8 +58,7 @@ type Data struct {
 	Drops map[string]uint64
 	// LocalityStats contains load reports per locality.
 	LocalityStats map[string]LocalityData
-	// ReportInternal is the duration since last time load was reported (Stats()
-	// was called).
+	// ReportInterval is the duration over which load was reported.
 	ReportInterval time.Duration
 }
 
@@ -96,7 +91,8 @@ type ServerLoadData struct {
 	Sum float64
 }
 
-// PerClusterReporter wraps the methods from the LoadStore that are used here.
+// PerClusterReporter defines the methods that the LoadStore uses to track
+// per-cluster load reporting data.
 type PerClusterReporter interface {
 	CallStarted(locality string)
 	CallFinished(locality string, err error)

xds/internal/clients/xdsclient/client.go

@@ -18,8 +18,7 @@
  *
  */
 
-// Package xdsclient provides an implementation of the xDS client to
-// communicate with xDS management servers.
+// Package xdsclient provides an xDS (Extensible Discovery Service) client.
 //
 // It allows applications to:
 //   - Create xDS client instances with in-memory configurations.
@@ -48,9 +47,11 @@ func New(config Config) (*XDSClient, error) {
 
 // WatchResource starts watching the specified resource.
 //
-// typeURL must be present in the XDSClient's configured ResourceTypes. The
-// ResourceType will be used to decode the matching resource when it is
-// received, and the watcher will be called with the result.
+// typeURL must be present in the XDSClient's configured ResourceTypes. If
+// typeURL is not present, watch will not be started. The ResourceType
+// obtained from the ResourceTypes against the typeURL will be used to decode
+// the matching resource when it is received, and the watcher will be called
+// with the result.
 //
 // Cancel cancels the watch and prevents future calls to the watcher.
 func (c *XDSClient) WatchResource(typeURL string, name string, watcher ResourceWatcher) (cancel func()) {

xds/internal/clients/xdsclient/config.go

@@ -22,13 +22,19 @@ import (
 	"google.golang.org/grpc/xds/internal/clients"
 )
 
-// A Config structure is used to configure an xDS client. After one has been
-// passed to an xDS function it must not be modified. A Config may be used;
-// the xDS package will also not modify it.
+// Config is used to configure an xDS client. After one has been passed to an
+// xDS function, it must not be modified. A Config may be used; the xDS package
+// will also not modify it.
 type Config struct {
-	// Servers specifies a list of xDS management servers to connect to,
-	// including fallbacks. xDS client use the first available server from the
-	// list. To ensure high availability, list the most reliable server first.
+	// Servers specifies a list of xDS management servers to connect to. The
+	// order of the servers in this list reflects the order of preference of
+	// the data returned by those servers. xDS client use the first
+	// available server from the list.
+	//
+	// See [gRFC A71] for more details on fallback behavior when the primary
+	// xDS server is unavailable.
+	//
+	// [gRFC A71]: https://github.com/grpc/proposal/blob/master/A71-xds-fallback.md
 	Servers []clients.ServerConfig
 
 	// Authorities map is used to define different authorities, in a federated

xds/internal/clients/xdsclient/resource_watcher.go

@@ -18,32 +18,26 @@
 
 package xdsclient
 
-// OnCallbackProcessed is a function to be invoked by resource watcher
-// implementations upon completing the processing of a callback from the xDS
-// client. Failure to invoke this callback prevents the xDS client from reading
-// further messages from the xDS server.
-type OnCallbackProcessed func()
-
-// ResourceDataOrError is a struct that contains either ResourceData or
-// error. It is used to represent the result of an xDS resource update. Exactly
-// one of Data or Err will be non-nil.
-type ResourceDataOrError struct {
-	Data ResourceData
-	Err  error
-}
-
 // ResourceWatcher wraps the callbacks to be invoked for different events
 // corresponding to the resource being watched. gRFC A88 contains an exhaustive
 // list of what method is invoked under what conditions.
+//
+// onCallbackProcessed in the callbacks is a function to be invoked by
+// resource watcher implementations upon completing the processing of a
+// callback from the xDS client. Failure to invoke this callback prevents the
+// xDS client from reading further messages from the xDS server.
 type ResourceWatcher interface {
-	// ResourceChanged either indicates a new version of the resource is
-	// available or an an error occurred while trying to fetch or decode the
-	// associated resource. In case of an error, the previous version of the
-	// resource should be considered invalid.
-	ResourceChanged(ResourceDataOrError, OnCallbackProcessed)
+	// ResourceChanged indicates a new version of the resource is available.
+	ResourceChanged(resourceData ResourceData, onCallbackProcessed func())
+
+	// ResourceError indicates an error occurred while trying to fetch or
+	// decode the associated resource. The previous version of the resource
+	// should be considered invalid.
+	ResourceError(err error, onCallbackProcessed func())
 
-	// AmbientError indicates an error occurred while trying to fetch or decode
-	// the associated resource.  The previous version of the resource should still
-	// be considered valid.
-	AmbientError(err error, done func())
+	// AmbientError indicates an error occurred after a resource has been
+	// received that should not modify the use of that resource but may be
+	// useful information about the ambient state of the XdsClient. The
+	// previous version of the resource should still be considered valid.
+	AmbientError(err error, onCallbackProcessed func())
 }