core: implement Channel State API for PickFirst #3300
Conversation
| if (subchannel == null) { | ||
| subchannel = helper.createSubchannel(newEag, Attributes.EMPTY); | ||
| helper.updatePicker(new Picker(PickResult.withSubchannel(subchannel))); | ||
| helper.updateBalancingState(CONNECTING, new Picker(PickResult.withSubchannel(subchannel))); |
There was a problem hiding this comment.
The new subchannel won't try to connect until an RPC is started.
The initial state of a new subchannel is IDLE. This should pass IDLE.
There was a problem hiding this comment.
The initial state of ManagedChannelImpl is IDLE. I think the method handleResolvedAddressGroups() wouldn't happen before NameResolver.start(), which wouldn't happen before ManagedChannelImpl.exitIdle(). So it should not be IDLE when this method is called.
| } | ||
| if (currentState == SHUTDOWN) { | ||
| // Either IDLE_TIMEOUT or channel is shutdown. Won't have any effect for the latter case. | ||
| helper.updateBalancingState(IDLE, new Picker(PickResult.withNoResult())); |
There was a problem hiding this comment.
IDLE_TIMEOUT is completely a channel impl logic. Even if we wanted to expose it to the application, it should be channel impl that would do that, i.e., transit to IDLE when IdleModeTimer expires and ignore updates from balancer if it's not the current one (which channel impl should be probably doing in the first place).
The balancer should not assume the channel's state. It should only provide state from its own perspective.
There was a problem hiding this comment.
Good point. Fixed
| nameResolver.shutdown(); | ||
| nameResolver = getNameResolver(target, nameResolverFactory, nameResolverParams); | ||
| loadBalancer.shutdown(); | ||
| loadBalancer = null; |
There was a problem hiding this comment.
In LbHelperImpl.updateBalancingState(), the state and the picker should be ignored if loadBalancer != lb, so that the channel ignores any updates from a balancer that has been shutdown and discarded.
There was a problem hiding this comment.
I don't like this code in ManagedChannelImpl:
LbHelperImpl helper = new LbHelperImpl(nameResolver);
helper.lb = loadBalancerFactory.newLoadBalancer(helper);
this.loadBalancer = helper.lb;
Because ManagedChannelImpl does not own loadBalancerFactory, it does not know the behavior of loadBalancerFactory.newLoadBalancer(), which may just always recycle the old loadBalancer and restart it so that loadBalancer == lb is always true.
There was a problem hiding this comment.
A factory that recycles the instances is violating the API. If the factory is shared among multiple channels, worse things will happen anyway. I don't think we should worry about it.
There was a problem hiding this comment.
If the factory is shared among multiple channels, it's not worse as long as the factory only recycles the completely shutdown ones.
What about letting ManagedChannelImpl have a helper field instead of loadBalancer field. And in LbHelperImpl, checking this == ManagedChannelImpl.this.helper instead of checking loadBalancer == lb?
| checkNotNull(newState, "newState"); | ||
| checkNotNull(newPicker, "newPicker"); | ||
|
|
||
| if (this != lbHelper) { |
There was a problem hiding this comment.
lbHelper must be accessed from channelExecutor.
There was a problem hiding this comment.
Corrected. PTAL
| @Deprecated | ||
| @Override | ||
| public void updatePicker(final SubchannelPicker picker) { | ||
| if (this != lbHelper) { |
|
retest this please |
2 participants
No description provided.