grpclb: split grpclb out of top level grpc package

[carl-mastrangelo]

This PR splits out grpclb from grpc. I have made the PR in several commits so you can see more clearly the steps that happened.

There are a few possibly contentious points that I would like to make clear up front:

• grpclb will no longer autoload as a load balancer. I think this is okay, as service config is not widely (at all?) used, and I believe this is the only way to access it.
• internal is used more, as a way of having code shared between packages without exposing types
• ConnectivityStateEvaluator, as used by grpclb, is no longer thread safe. I believe there is an outer mutex that guards access, but I want to point out this subtle change up here.

All but one tests pass with this, due to another cyclic dependency. I can fix this, but it is a little more widely scoped (such as exposing grpc.server and grpc.errorDesc in the internal package). This PR is a nearly-passing sample of that last step to get this working.

PTAL @menghanl @dfawley

[carl-mastrangelo]

@menghanl PTAL

There is now a fake grpclb used in the top level test.

Also, NewLBBuilderWithFallbackTimeout is no longer public. It is not public in Java, with the expectation of adding it in if necessary. The reason it is removed is because it is the only public method in the package which looks weird. Since no one is using it internally, or from searching on Godoc, I think it should be removed.

There is still some followup items (like docs), but I can do those in a followup PR. This one is getting too big.

[menghanl]

Thanks a lot for this change! Some nits/minor comments.

---

Review status: all files reviewed at latest revision, all discussions resolved.

---

balancer_switching_test.go, line 392 at r2 (raw file):

}

func installFakeGRPCLB() {

Delete this function?

---

balancer_v1_wrapper.go, line 83 at r2 (raw file):

	targetAddr string // Target without the scheme.

	// To aggregate the connectivity state.

Nit: move these three lines below mu to indicate that they are protected by the mutex.

---

balancer/balancer.go, line 230 at r2 (raw file):

}

// ConnectivityStateEvaluator gets updated by addrConns when their

How about renaming this to ConnectivityStateEvaluator?

And update the comment?

// ConnectivityStateEvaluator takes the connectivity states of multiple SubConns
// and returns one aggregated connectivity state.
//
// It's not thread safe.

---

balancer/balancer.go, line 239 at r2 (raw file):

}

// RecordTransition records state change happening in every subConn and based on

Also on comments, how about this:

// RecordTransition records state change happening in subConn and based on that
// it evaluates what aggregated state should be.
//
//  - If at least one SubConn in Ready, the aggregated state is Ready;
//  - Else if at least one SubConn in Connecting, the aggregated state is Connecting;
//  - Else the aggregated state is TransientFailure.
//
// Idle and Shutdown are not considered.

---

internal/internal.go, line 31 at r2 (raw file):

// set by clientconn.go
var (

Merge all three into one var ( ... ) block?

And comment that those two are only used by grpclb?

---

balancer/grpclb/grpclb_test.go, line 475 at r2 (raw file):

	var i int
	for i = 0; i < 1000; i++ {
		if _, err := testC.EmptyCall(ctx, &testpb.Empty{}, grpc.FailFast(false)); err == nil {

How about creating a new ctx with 2 seconds timeout before entering the loop?

---

balancer/grpclb/grpclb_test.go, line 484 at r2 (raw file):

	}
	select {
	case <-ctx.Done():

Why is this check necessary?

---

Comments from Reviewable

[carl-mastrangelo]

Reviewable is asking for overly broad permissions so comments here:

How about creating a new ctx with 2 seconds timeout before entering the loop?

I had at one point, but it needs a second cancel function, and the deadline is already encompassed by the prior one. If the call hangs (as it did in my testing) this could take 1000 * 2 seconds to complete, which is too long.

Why is this check necessary?

Because if the RPC hangs, the context will be done, but the iterator will not be 1000.

All other comments are "done"

[menghanl]

Review status: 12 of 13 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

balancer_v1_wrapper.go, line 91 at r4 (raw file):

	// - Build hasn't return, cc doesn't have access to balancer.
	startCh chan struct{}

Nit: to show mu also covers the following fields, I would remove this blank line.

---

Comments from Reviewable

[menghanl]

Thanks for the fixes.

Sorry I merged another grpclb PR #2101 that caused conflicts.

Three conflicts if I didn't miss anything:

• Fix import for grpclb.go
• Rename rpcStats to rpcStatsForTest in grpclb_test.go
• Also rename newRPCStats to newRPCStatsForTest in grpclb_test.go

[carl-mastrangelo]

@menghanl done! PTAL

[menghanl]

Thanks a lot! LGTM.