Review comments #1.

easwars · Apr 28, 2020 · 302134e · 302134e
1 parent 4dbdf73
commit 302134e
Show file tree

Hide file tree

Showing 7 changed files with 16 additions and 30 deletions.
diff --git a/balancer/rls/internal/balancer.go b/balancer/rls/internal/balancer.go
@@ -22,7 +22,6 @@ package rls
 
 import (
 	"sync"
-	"time"
 
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/balancer"
@@ -37,9 +36,7 @@ var (
 	_ balancer.V2Balancer = (*rlsBalancer)(nil)
 
 	// For overriding in tests.
-	newRLSClientFunc = func(cc *grpc.ClientConn, target string, timeout time.Duration) *rlsClient {
-		return newRLSClient(cc, target, timeout)
-	}
+	newRLSClientFunc = newRLSClient
 )
 
 // rlsBalancer implements the RLS LB policy.
@@ -65,7 +62,8 @@ type rlsBalancer struct {
 // the update will happen asynchronously.
 func (lb *rlsBalancer) run() {
 	for {
-		// TODO(easwars): Handle other updates.
+		// TODO(easwars): Handle other updates like subConn state changes, RLS
+		// responses from the server etc.
 		select {
 		case u := <-lb.ccUpdateCh:
 			lb.handleClientConnUpdate(u)
@@ -90,14 +88,7 @@ func (lb *rlsBalancer) handleClientConnUpdate(ccs *balancer.ClientConnState) {
 	}
 
 	newCfg := ccs.BalancerConfig.(*lbConfig)
-	oldCfg := lb.lbCfg
-	if oldCfg == nil {
-		// This is the first time we are receiving a service config.
-		lb.lbCfg = &lbConfig{}
-		oldCfg = lb.lbCfg
-	}
-
-	if oldCfg.Equal(newCfg) {
+	if lb.lbCfg.Equal(newCfg) {
 		grpclog.Info("rls: new service config matches existing config")
 		return
 	}
@@ -121,11 +112,12 @@ func (lb *rlsBalancer) UpdateClientConnState(ccs balancer.ClientConnState) error
 func (lb *rlsBalancer) ResolverError(error) {
 	// ResolverError is called by gRPC when the name resolver reports an error.
 	// TODO(easwars): How do we handle this?
+	grpclog.Fatal("rls: ResolverError is not yet unimplemented")
 }
 
 // UpdateSubConnState implements balancer.V2Balancer interface.
 func (lb *rlsBalancer) UpdateSubConnState(_ balancer.SubConn, _ balancer.SubConnState) {
-	grpclog.Error("rlsbalancer.UpdateSubConnState is not yet implemented")
+	grpclog.Fatal("rls: UpdateSubConnState is not yet implemented")
 }
 
 // Cleans up the resources allocated by the LB policy including the clientConn
@@ -143,12 +135,12 @@ func (lb *rlsBalancer) Close() {
 
 // HandleSubConnStateChange implements balancer.Balancer interface.
 func (lb *rlsBalancer) HandleSubConnStateChange(_ balancer.SubConn, _ connectivity.State) {
-	grpclog.Errorf("UpdateSubConnState should be called instead of HandleSubConnStateChange")
+	grpclog.Fatal("UpdateSubConnState should be called instead of HandleSubConnStateChange")
 }
 
 // HandleResolvedAddrs implements balancer.Balancer interface.
 func (lb *rlsBalancer) HandleResolvedAddrs(_ []resolver.Address, _ error) {
-	grpclog.Errorf("UpdateClientConnState should be called instead of HandleResolvedAddrs")
+	grpclog.Fatal("UpdateClientConnState should be called instead of HandleResolvedAddrs")
 }
 
 // updateControlChannel updates the RLS client if required.

diff --git a/balancer/rls/internal/balancer_test.go b/balancer/rls/internal/balancer_test.go
@@ -27,10 +27,10 @@ import (
 
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/balancer"
-	"google.golang.org/grpc/balancer/rls/internal/testutils"
 	"google.golang.org/grpc/balancer/rls/internal/testutils/fakeserver"
 	"google.golang.org/grpc/credentials"
 	"google.golang.org/grpc/internal/grpctest"
+	"google.golang.org/grpc/internal/testutils"
 	"google.golang.org/grpc/testdata"
 )
 

diff --git a/balancer/rls/internal/builder.go b/balancer/rls/internal/builder.go
@@ -47,6 +47,7 @@ func (*rlsBB) Build(cc balancer.ClientConn, opts balancer.BuildOptions) balancer
 		done:       grpcsync.NewEvent(),
 		cc:         cc,
 		opts:       opts,
+		lbCfg:      &lbConfig{},
 		ccUpdateCh: make(chan *balancer.ClientConnState),
 	}
 	go lb.run()

diff --git a/balancer/rls/internal/client_test.go b/balancer/rls/internal/client_test.go
@@ -28,15 +28,12 @@ import (
 
 	"github.com/golang/protobuf/proto"
 	"github.com/google/go-cmp/cmp"
+	"google.golang.org/grpc"
 	rlspb "google.golang.org/grpc/balancer/rls/internal/proto/grpc_lookup_v1"
-
+	"google.golang.org/grpc/balancer/rls/internal/testutils/fakeserver"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/internal/testutils"
 	"google.golang.org/grpc/status"
-
-	"google.golang.org/grpc/balancer/rls/internal/testutils"
-
-	"google.golang.org/grpc"
-	"google.golang.org/grpc/balancer/rls/internal/testutils/fakeserver"
 )
 
 const (

diff --git a/balancer/rls/internal/picker_test.go b/balancer/rls/internal/picker_test.go
@@ -28,15 +28,13 @@ import (
 	"testing"
 	"time"
 
-	"google.golang.org/grpc/balancer/rls/internal/testutils"
-
-	"google.golang.org/grpc/internal/grpcrand"
-
 	"github.com/google/go-cmp/cmp"
 	"google.golang.org/grpc/balancer"
 	"google.golang.org/grpc/balancer/rls/internal/cache"
 	"google.golang.org/grpc/balancer/rls/internal/keys"
 	rlspb "google.golang.org/grpc/balancer/rls/internal/proto/grpc_lookup_v1"
+	"google.golang.org/grpc/internal/grpcrand"
+	"google.golang.org/grpc/internal/testutils"
 	"google.golang.org/grpc/metadata"
 )
 

diff --git a/balancer/rls/internal/testutils/fakeserver/fakeserver.go b/balancer/rls/internal/testutils/fakeserver/fakeserver.go
@@ -27,11 +27,10 @@ import (
 	"net"
 	"time"
 
-	"google.golang.org/grpc/balancer/rls/internal/testutils"
-
 	"google.golang.org/grpc"
 	rlsgrpc "google.golang.org/grpc/balancer/rls/internal/proto/grpc_lookup_v1"
 	rlspb "google.golang.org/grpc/balancer/rls/internal/proto/grpc_lookup_v1"
+	"google.golang.org/grpc/internal/testutils"
 )
 
 const (

diff --git a/balancer/rls/internal/testutils/channel.go → internal/testutils/channel.go b/balancer/rls/internal/testutils/channel.go → internal/testutils/channel.go
@@ -15,7 +15,6 @@
  * limitations under the License.
  */
 
-// Package testutils provides utility types, for use in tests.
 package testutils
 
 import (