xds: implement WeightedTargetLoadBalancer #6731
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,205 @@ | ||
| /* | ||
| * 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.annotations.VisibleForTesting; | ||
| 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.WeightedRandomPicker.WeightedPickerFactory; | ||
| 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 final WeightedPickerFactory weightedPickerFactory; | ||
|
|
||
| private Map<String, WeightedPolicySelection> targets = ImmutableMap.of(); | ||
|
|
||
| WeightedTargetLoadBalancer(Helper helper) { | ||
| this( | ||
| checkNotNull(helper, "helper"), | ||
| WeightedRandomPicker.RANDOM_PICKER_FACTORY); | ||
|
There was a problem hiding this comment. 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. |
||
| } | ||
|
|
||
| @VisibleForTesting | ||
| WeightedTargetLoadBalancer( | ||
| Helper helper, WeightedPickerFactory weightedPickerFactory) { | ||
| this.helper = helper; | ||
| this.weightedPickerFactory = weightedPickerFactory; | ||
| 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 = weightedPickerFactory.picker(childPickers); | ||
| } | ||
|
|
||
| if (overallState != null) { | ||
| helper.updateBalancingState(overallState, picker); | ||
| } | ||
| } | ||
|
|
||
| @Nullable | ||
| private ConnectivityState aggregateState( | ||
|
There was a problem hiding this comment. 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. 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; | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
WeightedPickerFactoryinstance 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
WeightedRandomPickerbut 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.
WeightedPickerFactorydoes not claim to test anything thatWeightedRandomPickerdoes. WhatWeightedRandomPickerdoes is well-tested in its own test with a fake random. UsingWeightedPickerFactoryinLocalityStoreTest/WeightedTargetLoadBalancerTestis to avoid re-testingWeightedRandomPicker. Passing a fake random instead of the factory toLocalityStore/WeightedTargetLoadBalancerin 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
Auses classBwhereBis well-tested with its ownBTest. ThenATestwill just useBFactory.BFactorydoes not claim to test anything ofB.ATestdoes not claim to test any implementation detail encapsulated inBUsingB's constructor arg instead ofBFactoryinATest, you are retestingBagain.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
EdsLoadBalanceris 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
Bis heavy (i.e., small interface with large implementation).WeightedRandomPickeris completely an internal implementation ofWeightedTargetLoadBalancer/LocalityStore, even though you write it as a modularized subcomponent. It's better even ifWeightedRandomPickeris 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...