balancer: Enforce embedding the SubConn interface in implementations

[arjan-bal]

Presently, the doc comment on the balancer.SubConn interface mentions that external implementers should embed the balancer.SubConn interface, however, this is not enforced at compilation time:

// This interface is to be implemented by gRPC. Users should not need their own
// implementation of this interface. For situations like testing, any
// implementations should embed this interface. This allows gRPC to add new
// methods to this interface.

This PR adds an unexported method on the interface to add a compile time check for enforcing the requirement.

RELEASE NOTES:

• balancer: An unexported method is added to the balancer.SubConn interface to force implementors to embed the interface. This requirement is present in the interface documentation, but wasn't enforced earlier.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 81.81%. Comparing base (d66fc3a) to head (152cbce). Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7758      +/-   ##
==========================================
+ Coverage   81.71%   81.81%   +0.09%     
==========================================
  Files         374      371       -3     
  Lines       38166    37700     -466     
==========================================
- Hits        31188    30844     -344     
+ Misses       5699     5563     -136     
- Partials     1279     1293      +14     

Files with missing lines	Coverage Δ
balancer/balancer.go	96.77% <ø> (ø)
balancer_wrapper.go	84.03% <ø> (ø)
internal/testutils/balancer.go	84.32% <ø> (ø)

... and 33 files with indirect coverage changes

[arjan-bal]

This is a potential breaking change. I suspect it would only break tests (doing mocking?), but it's possible to break actual users. That's OK, we just need to be careful to document the change and also take care when importing the code internally.

@dfawley , I checked the internal usages and found that adding a private method will break code generated using GoMock, e.g: https://github.com/GoogleCloudPlatform/grpc-gcp-go/blob/d2bc599b72aeef2d85143b234ca7d602ae7f4302/grpcgcp/mocks/mock_balancer.go#L116-L119

Due to the private method, the generated mock no longer implements the balancer.SubConn interface, even though the mock has a private method with the same signature.

Users could manually edit the generated mocks to embed balancer.SubConn. Do you think it's okay to expect users manually edit their mocks?

[easwars]

I checked the internal usages and found that adding a private method will break code generated using GoMock

Hmm.. at least internally, mocking is not recommended for Go. I'm not sure about the external world though. Do you have any experience using mocking frameworks and know what is more commonly used and whether they generate code differently wherein the generated code embeds the mocked interface? (some google searching didn't yield anything useful)

Users could manually edit the generated mocks to embed balancer.SubConn. Do you think it's okay to expect users manually edit their mocks?

I guess this is no worse if you didn't use mocking and didn't embed, but chose to implement the methods to satisfy the interface.

[dfawley]

Do you think it's okay to expect users manually edit their mocks?

Probably not if they're generated, unless they're also checked in, and then...maybe. How many instances of this exist? Is it feasible to fix all the ones that we're (indirectly) responsible for?

[arjan-bal]

The PR in grpc-gcp-go is merged: https://github.com/GoogleCloudPlatform/grpc-gcp-go/pull/89 grpc-gcp-go may need to do a release before we're clear to merge this PR.