-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
xds: implement WeightedTargetLoadBalancer #6731
Changes from all commits
2cf623e
94ca8f4
40d402a
5014be7
c847c31
dfa75cb
ca6c100
3c33dac
c22b823
9b4ed3d
ecc6042
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,193 @@ | ||
/* | ||
* Copyright 2020 The gRPC Authors | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package io.grpc.xds; | ||
|
||
import static com.google.common.base.Preconditions.checkNotNull; | ||
import static io.grpc.ConnectivityState.CONNECTING; | ||
import static io.grpc.ConnectivityState.IDLE; | ||
import static io.grpc.ConnectivityState.READY; | ||
import static io.grpc.ConnectivityState.TRANSIENT_FAILURE; | ||
import static io.grpc.xds.XdsSubchannelPickers.BUFFER_PICKER; | ||
|
||
import com.google.common.collect.ImmutableMap; | ||
import io.grpc.ConnectivityState; | ||
import io.grpc.InternalLogId; | ||
import io.grpc.LoadBalancer; | ||
import io.grpc.Status; | ||
import io.grpc.util.ForwardingLoadBalancerHelper; | ||
import io.grpc.util.GracefulSwitchLoadBalancer; | ||
import io.grpc.xds.WeightedRandomPicker.WeightedChildPicker; | ||
import io.grpc.xds.WeightedTargetLoadBalancerProvider.WeightedPolicySelection; | ||
import io.grpc.xds.WeightedTargetLoadBalancerProvider.WeightedTargetConfig; | ||
import io.grpc.xds.XdsLogger.XdsLogLevel; | ||
import io.grpc.xds.XdsSubchannelPickers.ErrorPicker; | ||
import java.util.ArrayList; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
import javax.annotation.Nullable; | ||
|
||
/** Load balancer for weighted_target policy. */ | ||
final class WeightedTargetLoadBalancer extends LoadBalancer { | ||
|
||
private final XdsLogger logger; | ||
private final Map<String, GracefulSwitchLoadBalancer> childBalancers = new HashMap<>(); | ||
private final Map<String, ChildHelper> childHelpers = new HashMap<>(); | ||
private final Helper helper; | ||
|
||
private Map<String, WeightedPolicySelection> targets = ImmutableMap.of(); | ||
|
||
WeightedTargetLoadBalancer(Helper helper) { | ||
this.helper = helper; | ||
logger = XdsLogger.withLogId( | ||
InternalLogId.allocate("weighted-target-lb", helper.getAuthority())); | ||
logger.log(XdsLogLevel.INFO, "Created"); | ||
} | ||
|
||
@Override | ||
public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { | ||
logger.log(XdsLogLevel.DEBUG, "Received resolution result: {0}", resolvedAddresses); | ||
Object lbConfig = resolvedAddresses.getLoadBalancingPolicyConfig(); | ||
checkNotNull(lbConfig, "missing weighted_target lb config"); | ||
|
||
WeightedTargetConfig weightedTargetConfig = (WeightedTargetConfig) lbConfig; | ||
Map<String, WeightedPolicySelection> newTargets = weightedTargetConfig.targets; | ||
|
||
for (String targetName : newTargets.keySet()) { | ||
WeightedPolicySelection weightedChildLbConfig = newTargets.get(targetName); | ||
if (!targets.containsKey(targetName)) { | ||
ChildHelper childHelper = new ChildHelper(); | ||
GracefulSwitchLoadBalancer childBalancer = new GracefulSwitchLoadBalancer(childHelper); | ||
childBalancer.switchTo(weightedChildLbConfig.policySelection.getProvider()); | ||
childHelpers.put(targetName, childHelper); | ||
childBalancers.put(targetName, childBalancer); | ||
} else if (!weightedChildLbConfig.policySelection.getProvider().equals( | ||
targets.get(targetName).policySelection.getProvider())) { | ||
childBalancers.get(targetName) | ||
.switchTo(weightedChildLbConfig.policySelection.getProvider()); | ||
} | ||
} | ||
|
||
targets = newTargets; | ||
|
||
for (String targetName : targets.keySet()) { | ||
childBalancers.get(targetName).handleResolvedAddresses( | ||
resolvedAddresses.toBuilder() | ||
.setLoadBalancingPolicyConfig(targets.get(targetName).policySelection.getConfig()) | ||
.build()); | ||
} | ||
|
||
// Cleanup removed targets. | ||
// TODO(zdapeng): cache removed target for 15 minutes. | ||
for (String targetName : childBalancers.keySet()) { | ||
if (!targets.containsKey(targetName)) { | ||
childBalancers.get(targetName).shutdown(); | ||
} | ||
} | ||
childBalancers.keySet().retainAll(targets.keySet()); | ||
childHelpers.keySet().retainAll(targets.keySet()); | ||
} | ||
|
||
@Override | ||
public void handleNameResolutionError(Status error) { | ||
logger.log(XdsLogLevel.WARNING, "Received name resolution error: {0}", error); | ||
if (childBalancers.isEmpty()) { | ||
helper.updateBalancingState(TRANSIENT_FAILURE, new ErrorPicker(error)); | ||
} | ||
for (LoadBalancer childBalancer : childBalancers.values()) { | ||
childBalancer.handleNameResolutionError(error); | ||
} | ||
} | ||
|
||
@Override | ||
public boolean canHandleEmptyAddressListFromNameResolution() { | ||
return true; | ||
} | ||
|
||
@Override | ||
public void shutdown() { | ||
logger.log(XdsLogLevel.INFO, "Shutdown"); | ||
for (LoadBalancer childBalancer : childBalancers.values()) { | ||
childBalancer.shutdown(); | ||
} | ||
} | ||
|
||
private void updateOverallBalancingState() { | ||
List<WeightedChildPicker> childPickers = new ArrayList<>(); | ||
|
||
ConnectivityState overallState = null; | ||
for (String name : targets.keySet()) { | ||
ChildHelper childHelper = childHelpers.get(name); | ||
ConnectivityState childState = childHelper.currentState; | ||
overallState = aggregateState(overallState, childState); | ||
if (READY == childState) { | ||
int weight = targets.get(name).weight; | ||
childPickers.add(new WeightedChildPicker(weight, childHelper.currentPicker)); | ||
} | ||
} | ||
|
||
SubchannelPicker picker; | ||
if (childPickers.isEmpty()) { | ||
if (overallState == TRANSIENT_FAILURE) { | ||
picker = new ErrorPicker(Status.UNAVAILABLE); // TODO: more details in status | ||
} else { | ||
picker = XdsSubchannelPickers.BUFFER_PICKER; | ||
} | ||
} else { | ||
picker = new WeightedRandomPicker(childPickers); | ||
} | ||
|
||
if (overallState != null) { | ||
helper.updateBalancingState(overallState, picker); | ||
} | ||
} | ||
|
||
@Nullable | ||
private ConnectivityState aggregateState( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is related to #6650. Might do it after the design in general is clear. |
||
@Nullable ConnectivityState overallState, ConnectivityState childState) { | ||
if (overallState == null) { | ||
return childState; | ||
} | ||
if (overallState == READY || childState == READY) { | ||
return READY; | ||
} | ||
if (overallState == CONNECTING || childState == CONNECTING) { | ||
return CONNECTING; | ||
} | ||
if (overallState == IDLE || childState == IDLE) { | ||
return IDLE; | ||
} | ||
return overallState; | ||
} | ||
|
||
private final class ChildHelper extends ForwardingLoadBalancerHelper { | ||
ConnectivityState currentState = CONNECTING; | ||
SubchannelPicker currentPicker = BUFFER_PICKER; | ||
|
||
@Override | ||
public void updateBalancingState(ConnectivityState newState, SubchannelPicker newPicker) { | ||
currentState = newState; | ||
currentPicker = newPicker; | ||
updateOverallBalancingState(); | ||
} | ||
|
||
@Override | ||
protected Helper delegate() { | ||
return helper; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 whosepicker(...)
method creates a fakeSubchannelPicker
. 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WeightedPickerFactory
does not claim to test anything thatWeightedRandomPicker
does. WhatWeightedRandomPicker
does is well-tested in its own test with a fake random. UsingWeightedPickerFactory
inLocalityStoreTest
/WeightedTargetLoadBalancerTest
is to avoid re-testingWeightedRandomPicker
. 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 ofWeightedRandomPicker
.We have argued about this a lot of times. When class
A
uses classB
whereB
is well-tested with its ownBTest
. ThenATest
will just useBFactory
.BFactory
does not claim to test anything ofB
.ATest
does not claim to test any implementation detail encapsulated inB
UsingB
's constructor arg instead ofBFactory
inATest
, you are retestingB
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 useWeightedTargetLoadBalancer
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofWeightedTargetLoadBalancer
/LocalityStore
, even though you write it as a modularized subcomponent. It's better even ifWeightedRandomPicker
is inlined inWeightedTargetLoadBalancer
/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 ofWeigtedTargetLoadBalancer
/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...