review comments

test/pickfirst_test.go

@@ -382,6 +382,7 @@ func (s) TestPickFirst_StickyTransientFailure(t *testing.T) {
 	wg.Wait()
 }
 
+// Tests the PF LB policy with shuffling enabled.
 func (s) TestPickFirst_ShuffleAddressList(t *testing.T) {
 	defer func(old bool) { envconfig.PickFirstLBConfig = old }(envconfig.PickFirstLBConfig)
 	envconfig.PickFirstLBConfig = true
@@ -393,6 +394,7 @@ func (s) TestPickFirst_ShuffleAddressList(t *testing.T) {
 	grpcrand.Shuffle = func(n int, f func(int, int)) {
 		if n != 2 {
 			t.Errorf("Shuffle called with n=%v; want 2", n)
+			return
 		}
 		f(0, 1) // reverse the two addresses
 	}
@@ -435,6 +437,8 @@ func (s) TestPickFirst_ShuffleAddressList(t *testing.T) {
 	}
 }
 
+// Tests the PF LB policy with the environment variable support of address list
+// shuffling disabled.
 func (s) TestPickFirst_ShuffleAddressListDisabled(t *testing.T) {
 	defer func(old bool) { envconfig.PickFirstLBConfig = old }(envconfig.PickFirstLBConfig)
 	envconfig.PickFirstLBConfig = false
@@ -446,6 +450,7 @@ func (s) TestPickFirst_ShuffleAddressListDisabled(t *testing.T) {
 	grpcrand.Shuffle = func(n int, f func(int, int)) {
 		if n != 2 {
 			t.Errorf("Shuffle called with n=%v; want 2", n)
+			return
 		}
 		f(0, 1) // reverse the two addresses
 	}
@@ -457,15 +462,10 @@ func (s) TestPickFirst_ShuffleAddressListDisabled(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 	defer cancel()
 
-	// Push an update with both addresses and shuffling disabled.  We should
-	// connect to backend 0.
-	r.UpdateState(resolver.State{Addresses: []resolver.Address{addrs[0], addrs[1]}})
-	if err := pickfirst.CheckRPCsToBackend(ctx, cc, addrs[0]); err != nil {
-		t.Fatal(err)
-	}
-
 	// Send a config with shuffling enabled.  This will reverse the addresses,
-	// but the channel should still be connected to backend 0.
+	// so we should connect to backend 1 if shuffling is supported.  However
+	// with it disabled at the start of the test, we will connect to backend 0
+	// instead.
 	shufState := resolver.State{
 		ServiceConfig: parseServiceConfig(t, r, serviceConfig),
 		Addresses:     []resolver.Address{addrs[0], addrs[1]},
@@ -474,18 +474,4 @@ func (s) TestPickFirst_ShuffleAddressListDisabled(t *testing.T) {
 	if err := pickfirst.CheckRPCsToBackend(ctx, cc, addrs[0]); err != nil {
 		t.Fatal(err)
 	}
-
-	// Send a resolver update with no addresses. This should push the channel
-	// into TransientFailure.
-	r.UpdateState(resolver.State{})
-	awaitState(ctx, t, cc, connectivity.TransientFailure)
-
-	// Send the same config as last time with shuffling enabled.  Since we are
-	// not connected to backend 0, we should connect to backend 1 if shuffling
-	// is supported.  However with it disabled at the start of the test, we
-	// will connect to backend 0 instead.
-	r.UpdateState(shufState)
-	if err := pickfirst.CheckRPCsToBackend(ctx, cc, addrs[0]); err != nil {
-		t.Fatal(err)
-	}
 }

xds/internal/xdsclient/xdslbregistry/converter/converter.go

@@ -28,7 +28,9 @@ import (
 	"strings"
 
 	"github.com/golang/protobuf/proto"
+	"google.golang.org/grpc"
 	"google.golang.org/grpc/balancer"
+	"google.golang.org/grpc/balancer/roundrobin"
 	"google.golang.org/grpc/balancer/weightedroundrobin"
 	"google.golang.org/grpc/internal/envconfig"
 	internalserviceconfig "google.golang.org/grpc/internal/serviceconfig"
@@ -110,11 +112,11 @@ func convertPickFirstProtoToServiceConfig(rawProto []byte, _ int) (json.RawMessa
 	if err != nil {
 		return nil, fmt.Errorf("error marshaling JSON for type %T: %v", pfCfg, err)
 	}
-	return makeBalancerConfigJSON("pick_first", js), nil
+	return makeBalancerConfigJSON(grpc.PickFirstBalancerName, js), nil
 }
 
 func convertRoundRobinProtoToServiceConfig([]byte, int) (json.RawMessage, error) {
-	return makeBalancerConfigJSON("round_robin", json.RawMessage("{}")), nil
+	return makeBalancerConfigJSON(roundrobin.Name, json.RawMessage("{}")), nil
 }
 
 type wrrLocalityLBConfig struct {

xds/internal/xdsclient/xdslbregistry/xdslbregistry_test.go

@@ -106,7 +106,7 @@ func (s) TestConvertToServiceConfigSuccess(t *testing.T) {
 			wantConfig: `[{"ring_hash_experimental": { "minRingSize": 10, "maxRingSize": 100 }}]`,
 		},
 		{
-			name: "pick_first",
+			name: "pick_first_shuffle",
 			policy: &v3clusterpb.LoadBalancingPolicy{
 				Policies: []*v3clusterpb.LoadBalancingPolicy_Policy{
 					{
@@ -120,6 +120,19 @@ func (s) TestConvertToServiceConfigSuccess(t *testing.T) {
 			},
 			wantConfig: `[{"pick_first": { "shuffleAddressList": true }}]`,
 		},
+		{
+			name: "pick_first",
+			policy: &v3clusterpb.LoadBalancingPolicy{
+				Policies: []*v3clusterpb.LoadBalancingPolicy_Policy{
+					{
+						TypedExtensionConfig: &v3corepb.TypedExtensionConfig{
+							TypedConfig: testutils.MarshalAny(&v3pickfirstpb.PickFirst{}),
+						},
+					},
+				},
+			},
+			wantConfig: `[{"pick_first": { "shuffleAddressList": false }}]`,
+		},
 		{
 			name: "round_robin",
 			policy: &v3clusterpb.LoadBalancingPolicy{
@@ -301,6 +314,8 @@ func (s) TestConvertToServiceConfigSuccess(t *testing.T) {
 			if err != nil {
 				t.Fatalf("ConvertToServiceConfig(%s) failed: %v", pretty.ToJSON(test.policy), err)
 			}
+			// got and want must be unmarshalled since JSON strings shouldn't
+			// generally be directly compared.
 			var got []map[string]interface{}
 			if err := json.Unmarshal(rawJSON, &got); err != nil {
 				t.Fatalf("Error unmarshalling rawJSON (%q): %v", rawJSON, err)