Subchannel identity crisis

[zhangkun83]

Since we are doing lot of wrapping on the LoadBalancer API, and Subchannels are passed through various interfaces, a wrapped Subchannel will require a lot of care to make sure it's consistent on all APIs, e.g.,

1. The Subchannel from createSubchannel() should be the same objects that's passed to SubchannelStateListener on every wrapping level.
2. An upcoming ORCA load reporting API is another example (xds: implement lb policy backend metric api #5639 (comment)).
3. The Subchannel from PickResult will have to be unwrapped as well. ManagedChannelImpl assumes the Subchannel from PickResult is the real "SubchannelImpl".

Possible solutions:

1. Move LoadBalancer.Helper methods that accepts Subchannel to the Subchannel class, e.g., updateSubchannelAddresses(). Stop passing Subchannel to the listeners. But that requires a start(), which is a non-trivial API change on the current Subchannel, but could be considered for new APIs.
2. The opposite of above -- move all state-mutating methods, e.g., shutdown(), requestConnection() to Helper, eliminating the need for wrapping Subchannel.
3. Provide a util to help correctly wrap Subchannel while keeping the consistency.
4. Add another identity object to Subchannel. This could also used by ManagedChannelImpl to retrieve the real "SubchannelImpl" from PickResult.

Solutions 2 and 5 wouldn't go along with our plan of changing Subchannel attributes to an object, because a wrapping Helper can only add its own stuff on that object by wrapping it, which ends up the need for unwrapping it on the Subchannel. Otherwise they seem to work well.

@ejona86 @voidzcy

[zhangkun83]

@ejona86 I have added Solution 2 which we didn't discuss yesterday. I think it makes a more practical sense, because it consolidates the points of interception -- previously a parent LB may need to wrap both Helper and Subchannel if it wants to intercept the Subchannel lifecycles. With Solution 2, only Helper needs to be wrapped, which would be a lot simpler. On the flip side, it only gets along with the current Subchannel.getAttributes(), not with our plan of changing it to Object Subchannel.getAttachment().

[zhangkun83]

/cc @dapengzhang0, your cached Subchannel Helper, which wraps Subchannel to intercept shutdown(), will be affected.

[ejona86]

Places we cast internally:

grpc-java/core/src/main/java/io/grpc/internal/GrpcUtil.java

Lines 688 to 692 in 026e4c5

	static ClientTransport getTransportFromPickResult(PickResult result, boolean isWaitForReady) {
	final ClientTransport transport;
	Subchannel subchannel = result.getSubchannel();
	if (subchannel != null) {
	transport = ((AbstractSubchannel) subchannel).obtainActiveTransport();

grpc-java/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

Lines 1201 to 1207 in 63f636b

	@Override
	public void updateSubchannelAddresses(
	LoadBalancer.Subchannel subchannel, List<EquivalentAddressGroup> addrs) {
	checkArgument(subchannel instanceof SubchannelImpl,
	"subchannel must have been returned from createSubchannel");
	((SubchannelImpl) subchannel).subchannel.updateAddresses(addrs);
	}

[zhangkun83]

Plan per offline discussion with @ejona86 and @voidzcy:

General idea is to stop passing Subchannel as arguments on all interfaces.

1. Revert SubchannelStateListener changes for 1.21.
2. Add start(StateListener) to Subchannel, where the StateListener doesn’t pass Subchannel
3. Drop the plan for Subchnanel.getObject(). If a LoadBalancer implementation wants to associate a custom object with Subchannel and access it from the listener, it can create the object between createSubchannel() and Subchannel.start().
4. CreateSubchannelArgs doesn’t take StateListener
5. Add a getter on Subchannel to get an object representing the underlying Subchannel/Transports. It may return an Object or marker interface
6. Move updateAddresses() to Subchannel
7. Drop Subchannel from OrcaOobListener

TBD:

1. Is StateListener interface or abstract class
2. The name of the getter of underlying object on Subchannel