xds: implement WeightedTargetLoadBalancer

[dapengzhang0]

• Implement weighted_target lb part of go/grpc-xds-traffic-splitting-and-path-matching
• Refactor InterLocalityPicker to reuse the RandomWeightedPicker.

[voidzcy]

We may want to make a utility method for this, as it is used in a couple of places (for any load balancers that contain a bunch of child balancers).

[dapengzhang0]

This is related to #6650. Might do it after the design in general is clear.

[voidzcy]

This class structure does not help with testing at all, as in order to provide your own random number generator in test, you have to implement a fake WeightedPickerFactory instance whose picker(...) method creates a fake SubchannelPicker. So everything about picking is fake. This tests nothing.

Instead, a better strategy is to not have this constructor at all. You implementation code should always call the constructor with injectable random number generator. In that way, your test code is still executing the real WeightedRandomPicker but only with the random number generator being faked out.

[dapengzhang0]

WeightedPickerFactory does not claim to test anything that WeightedRandomPicker does. What WeightedRandomPicker does is well-tested in its own test with a fake random. Using WeightedPickerFactory in LocalityStoreTest/WeightedTargetLoadBalancerTest is to avoid re-testing WeightedRandomPicker. Passing a fake random instead of the factory toLocalityStore/WeightedTargetLoadBalancer in test, you need recalculate the relation between the weights and the fake random number, that's basically following the implementation details of WeightedRandomPicker.

We have argued about this a lot of times. When class A uses class B where B is well-tested with its own BTest. Then ATest will just use BFactory. BFactory does not claim to test anything of B. ATest does not claim to test any implementation detail encapsulated in B Using B's constructor arg instead of BFactory in ATest, you are retesting B again.

Now I revised the class not to use a factory, but I'm still not going to retest the well-encapsulated and well-tested class before EdsLoadBalancer is refactored to use WeightedTargetLoadBalancer.

[voidzcy]

This test strategy is poor. It should be used only when the subcomponent B is heavy (i.e., small interface with large implementation). WeightedRandomPicker is completely an internal implementation of WeightedTargetLoadBalancer/LocalityStore, even though you write it as a modularized subcomponent. It's better even if WeightedRandomPicker is inlined in WeightedTargetLoadBalancer/LocalityStore. But in terms of code reuse, put it into a modularized class. Its implementation is not complex (even somewhat trivial), it's just using a random number generator to choose something in an input list. It is more like a utility. We are not keenly interested in testing the isolated behaviors of this sub-component. Instead, we are more interested in the real output of WeigtedTargetLoadBalancer/LocalityStore, which is the content of picker that eventually propagated to Channel (i.e., helper.updateBalancingState(State, Picker)).

The above is just my opinion. I am not going to block you on tests, as long as the class structure is obviously inappropriate. But really, existing tests test barely little real things...

[voidzcy]

This looks bad... You should rather call constructor to create a factory or call the factory class to get the instance. At least, not something that is not the factory itself.