xds: implement LRS LB policy - #6858
Conversation
| return PickResult.withSubchannel(subchannel); | ||
| } | ||
| }; | ||
| helper.updateBalancingState(ConnectivityState.READY, picker); |
There was a problem hiding this comment.
This is just testing test implementation. The subchannel of the picker and the action of calling childHelper.updateBalancingState() should be considered as test input instead of test implementation.
There was a problem hiding this comment.
This is the implicitly an "input", by my implementation choice: calling LrsLoadBalancer's handleResolvedAddresses(...) immediately triggers a subchannel become READY. This merges two steps into one: deliver addresses + deliver subchannel state. We do not care about how subchannels are created, so it's completely up to the fake balancer's implementation.
There was a problem hiding this comment.
Hiding implicit "input" in fake lb's handleResolvedAddresses() implementation makes the tests very difficult to read and surprises readers: you are testing against a very very special child balancer implementation.
You can make the input explicit by adding a method FakeLoadBalancer.deliverBalancingState()and call it explicitly in the test method - if you don't want to cache the childHelper and call childHelper. updateBalancingState() explicitly.
There was a problem hiding this comment.
How downstream balancer/helper propagate subchannel state/picker up to parent balancer is completely downstream balancer's logic. In this test, its choice is to immediately propagate a READY subchannel when it receives ResolvedAddresses.
makes the tests very difficult to read and surprises readers
Why this would surprise readers? Readers need to know how downstream balancer propagates subchannel state/picker to parent balancer, so it needs to read how this fake downstream balancer is implemented.
you are testing against a very very special child balancer implementation.
Does this matter to what is being tested and verified? It does not. From LrsLoadBalancer's perspective, downstream balancer is opaque, it's just a LoadBalancer.
There was a problem hiding this comment.
Why this would surprise readers?
A couple of reasons:
-
A fake object should normally have a trivial implementation, such as recording the input arguments and constructing a dummy return value. Fake object should normally not implement the overriding methods with any behaviors. Otherwise you should not name it fake object (sometimes I made the same mistake), but give them a meaningful name like
ImmediateReadyLoadBalancer,ImmediateReadyLbProvider, andimmediate-ready-policy-foo, and optionally provide javadoc for the behavior. Otherwise readers reading the test methods will ignore/overlook there is a behavior in the fake object implementation. -
Subchannel turning to READY state is triggered by external event: the transport is connected and notifies the subchannel state listener. Lb itself can not control when the event is fired.
CallingchildHelper.updateBalancingState(READY, picker)explicitly on demand in test methods as a test input is a sound way to simulate that the external event is happening. On the contrary, overridinghandleResolvedAddresses()to callchildHelper. updateBalancingState(READY, picker)is a weird behavior as if the lb itself fires the external event automatically.
Does this matter to what is being tested and verified?
Overriding handleResolvedAddresses() this way is technically equivalent to calling a dummy handleResolvedAddresses() and immediately calling childHelper.updateBalancingState(READY, picker) after it. If all the test methods only need to call the two together, technically there is no difference, but looks all the tests and future tests are relying on this special behavior. Two approaches can technically achieve the same thing but one is hacky.
Just my 2 cents.
There was a problem hiding this comment.
Sure, sounds solid. Thanks. Updated.
| ClientStreamTracer.Factory tracerFactory = result.getStreamTracerFactory(); | ||
| assertThat(((LoadRecordingStreamTracerFactory) tracerFactory).getCounter()) | ||
| .isSameInstanceAs(counter); | ||
| assertThat(result.getSubchannel().getAllAddresses()).isEqualTo(backendAddrs); |
There was a problem hiding this comment.
This line is just testing test implementation and nothing else is tested. Use mock subchannel with childHelper.updateBalancingState() as test input, and verify result.getSubchannel() is expected.
There was a problem hiding this comment.
Deleted. We only care about the picker, how subchannel are created with respect to addresses are completely up to child balancer/helper's implementation. Not interested by LrsLoadBalancer's behavior.
6db4a90 to
3c87ced
Compare
| public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { | ||
| List<EquivalentAddressGroup> addresses = resolvedAddresses.getAddresses(); | ||
| for (EquivalentAddressGroup eag : addresses) { | ||
| subchannels.put(eag, new NoopSubchannel(helper)); |
There was a problem hiding this comment.
This and NoopSubchannel seem to be a lot implementation. We don't really care the original resolved EADs (in general there's no relation between original EAG and subchannel, the EGAs could even be empty), and the subchannel can totally be mocked as long as we know childBalancer.helper. But this is fine.
There was a problem hiding this comment.
Yep, you are right. NoopSubchannel is mainly introduced for holding the reference to the Helper that creates it, which would be the instance that updateBalancingState(...) is invoked on. It's fine, all the required overridden methods are just no-op. Addresses are just used for a cleaner way of organizing/look up subchannels, to which states will be delivered. I agree, slightly overkill, but the way objects are organized looks pretty comfortable to me.
Part of xDS LB policy refactoring work. Implement the LRS LB policy for "balancing" endpoints within a certain locality.
2 participants
No description provided.