Skip to content

Commit 7136070

Browse files
ejona86ejona86
committed
util: Use acceptResolvedAddresses() for MultiChildLb children
A failing Status from acceptResolvedAddresses means something is wrong with the config, but parts of the config may still have been applied. Thus there are now two possible flows: errors that should prevent updateOverallBalancingState() and errors that should have no effect other than the return code. To manage that, MultChildLb must always be responsible for calling updateOverallBalancingState(). acceptResolvedAddressesInternal() was inlined to make that error processing easier. No existing usages actually needed to have logic between updating the children and regenerating the picker. RingHashLb already was verifying that the address list was not empty, so the short-circuiting when acceptResolvedAddressesInternal() returned an error was impossible to trigger. WrrLb's updateWeightTask() calls the last picker, so it can run before acceptResolvedAddressesInternal(); the only part that matters is re-creating the weightUpdateTimer.
1 parent a132123 commit 7136070

File tree

3 files changed

+66
-126
lines changed

3 files changed

+66
-126
lines changed

‎util/src/main/java/io/grpc/util/MultiChildLoadBalancer.java‎

Lines changed: 22 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -107,21 +107,22 @@ protected ChildLbState createChildLbState(Object key) {
107107
*/
108108
@Override
109109
public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
110+
logger.log(Level.FINE, "Received resolution result: {0}", resolvedAddresses);
110111
try {
111112
resolvingAddresses = true;
112113

113114
// process resolvedAddresses to update children
114-
AcceptResolvedAddrRetVal acceptRetVal = acceptResolvedAddressesInternal(resolvedAddresses);
115-
if (!acceptRetVal.status.isOk()) {
116-
return acceptRetVal.status;
115+
Map<Object, ResolvedAddresses> newChildAddresses = createChildAddressesMap(resolvedAddresses);
116+
117+
// Handle error case
118+
if (newChildAddresses.isEmpty()) {
119+
Status unavailableStatus = Status.UNAVAILABLE.withDescription(
120+
"NameResolver returned no usable address. " + resolvedAddresses);
121+
handleNameResolutionError(unavailableStatus);
122+
return unavailableStatus;
117123
}
118124

119-
// Update the picker and our connectivity state
120-
updateOverallBalancingState();
121-
122-
// shutdown removed children
123-
shutdownRemoved(acceptRetVal.removedChildren);
124-
return acceptRetVal.status;
125+
return updateChildrenWithResolvedAddresses(newChildAddresses);
125126
} finally {
126127
resolvingAddresses = false;
127128
}
@@ -149,31 +150,7 @@ public void shutdown() {
149150
childLbStates.clear();
150151
}
151152

152-
/**
153-
* This does the work to update the child map and calculate which children have been removed.
154-
* You must call {@link #updateOverallBalancingState} to update the picker
155-
* and call {@link #shutdownRemoved(List)} to shutdown the endpoints that have been removed.
156-
*/
157-
protected final AcceptResolvedAddrRetVal acceptResolvedAddressesInternal(
158-
ResolvedAddresses resolvedAddresses) {
159-
logger.log(Level.FINE, "Received resolution result: {0}", resolvedAddresses);
160-
161-
Map<Object, ResolvedAddresses> newChildAddresses = createChildAddressesMap(resolvedAddresses);
162-
163-
// Handle error case
164-
if (newChildAddresses.isEmpty()) {
165-
Status unavailableStatus = Status.UNAVAILABLE.withDescription(
166-
"NameResolver returned no usable address. " + resolvedAddresses);
167-
handleNameResolutionError(unavailableStatus);
168-
return new AcceptResolvedAddrRetVal(unavailableStatus, null);
169-
}
170-
171-
List<ChildLbState> removed = updateChildrenWithResolvedAddresses(newChildAddresses);
172-
return new AcceptResolvedAddrRetVal(Status.OK, removed);
173-
}
174-
175-
/** Returns removed children. */
176-
private List<ChildLbState> updateChildrenWithResolvedAddresses(
153+
private Status updateChildrenWithResolvedAddresses(
177154
Map<Object, ResolvedAddresses> newChildAddresses) {
178155
// Create a map with the old values
179156
Map<Object, ChildLbState> oldStatesMap =
@@ -183,6 +160,7 @@ private List<ChildLbState> updateChildrenWithResolvedAddresses(
183160
}
184161

185162
// Move ChildLbStates from the map to a new list (preserving the new map's order)
163+
Status status = Status.OK;
186164
List<ChildLbState> newChildLbStates = new ArrayList<>(newChildAddresses.size());
187165
for (Map.Entry<Object, ResolvedAddresses> entry : newChildAddresses.entrySet()) {
188166
ChildLbState childLbState = oldStatesMap.remove(entry.getKey());
@@ -191,21 +169,23 @@ private List<ChildLbState> updateChildrenWithResolvedAddresses(
191169
}
192170
newChildLbStates.add(childLbState);
193171
if (entry.getValue() != null) {
194-
childLbState.lb.handleResolvedAddresses(entry.getValue()); // update child LB
172+
// update child LB
173+
Status newStatus = childLbState.lb.acceptResolvedAddresses(entry.getValue());
174+
if (!newStatus.isOk()) {
175+
status = newStatus;
176+
}
195177
}
196178
}
197179

198180
childLbStates = newChildLbStates;
199-
// Remaining entries in map are orphaned
200-
return new ArrayList<>(oldStatesMap.values());
201-
}
181+
// Update the picker and our connectivity state
182+
updateOverallBalancingState();
202183

203-
protected final void shutdownRemoved(List<ChildLbState> removedChildren) {
204-
// Do shutdowns after updating picker to reduce the chance of failing an RPC by picking a
205-
// subchannel that has been shutdown.
206-
for (ChildLbState childLbState : removedChildren) {
184+
// Remaining entries in map are orphaned
185+
for (ChildLbState childLbState : oldStatesMap.values()) {
207186
childLbState.shutdown();
208187
}
188+
return status;
209189
}
210190

211191
@Nullable
@@ -406,14 +386,4 @@ public String toString() {
406386
return addrs.toString();
407387
}
408388
}
409-
410-
protected static class AcceptResolvedAddrRetVal {
411-
public final Status status;
412-
public final List<ChildLbState> removedChildren;
413-
414-
public AcceptResolvedAddrRetVal(Status status, List<ChildLbState> removedChildren) {
415-
this.status = status;
416-
this.removedChildren = removedChildren;
417-
}
418-
}
419389
}

‎xds/src/main/java/io/grpc/xds/RingHashLoadBalancer.java‎

Lines changed: 37 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -87,62 +87,46 @@ public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
8787
return addressValidityStatus;
8888
}
8989

90-
try {
91-
resolvingAddresses = true;
92-
AcceptResolvedAddrRetVal acceptRetVal = acceptResolvedAddressesInternal(resolvedAddresses);
93-
if (!acceptRetVal.status.isOk()) {
94-
return acceptRetVal.status;
95-
}
96-
97-
// Now do the ringhash specific logic with weights and building the ring
98-
RingHashConfig config = (RingHashConfig) resolvedAddresses.getLoadBalancingPolicyConfig();
99-
if (config == null) {
100-
throw new IllegalArgumentException("Missing RingHash configuration");
90+
// Now do the ringhash specific logic with weights and building the ring
91+
RingHashConfig config = (RingHashConfig) resolvedAddresses.getLoadBalancingPolicyConfig();
92+
if (config == null) {
93+
throw new IllegalArgumentException("Missing RingHash configuration");
94+
}
95+
Map<EquivalentAddressGroup, Long> serverWeights = new HashMap<>();
96+
long totalWeight = 0L;
97+
for (EquivalentAddressGroup eag : addrList) {
98+
Long weight = eag.getAttributes().get(XdsAttributes.ATTR_SERVER_WEIGHT);
99+
// Support two ways of server weighing: either multiple instances of the same address
100+
// or each address contains a per-address weight attribute. If a weight is not provided,
101+
// each occurrence of the address will be counted a weight value of one.
102+
if (weight == null) {
103+
weight = 1L;
101104
}
102-
Map<EquivalentAddressGroup, Long> serverWeights = new HashMap<>();
103-
long totalWeight = 0L;
104-
for (EquivalentAddressGroup eag : addrList) {
105-
Long weight = eag.getAttributes().get(XdsAttributes.ATTR_SERVER_WEIGHT);
106-
// Support two ways of server weighing: either multiple instances of the same address
107-
// or each address contains a per-address weight attribute. If a weight is not provided,
108-
// each occurrence of the address will be counted a weight value of one.
109-
if (weight == null) {
110-
weight = 1L;
111-
}
112-
totalWeight += weight;
113-
EquivalentAddressGroup addrKey = stripAttrs(eag);
114-
if (serverWeights.containsKey(addrKey)) {
115-
serverWeights.put(addrKey, serverWeights.get(addrKey) + weight);
116-
} else {
117-
serverWeights.put(addrKey, weight);
118-
}
105+
totalWeight += weight;
106+
EquivalentAddressGroup addrKey = stripAttrs(eag);
107+
if (serverWeights.containsKey(addrKey)) {
108+
serverWeights.put(addrKey, serverWeights.get(addrKey) + weight);
109+
} else {
110+
serverWeights.put(addrKey, weight);
119111
}
120-
// Calculate scale
121-
long minWeight = Collections.min(serverWeights.values());
122-
double normalizedMinWeight = (double) minWeight / totalWeight;
123-
// Scale up the number of hashes per host such that the least-weighted host gets a whole
124-
// number of hashes on the the ring. Other hosts might not end up with whole numbers, and
125-
// that's fine (the ring-building algorithm can handle this). This preserves the original
126-
// implementation's behavior: when weights aren't provided, all hosts should get an equal
127-
// number of hashes. In the case where this number exceeds the max_ring_size, it's scaled
128-
// back down to fit.
129-
double scale = Math.min(
130-
Math.ceil(normalizedMinWeight * config.minRingSize) / normalizedMinWeight,
131-
(double) config.maxRingSize);
132-
133-
// Build the ring
134-
ring = buildRing(serverWeights, totalWeight, scale);
135-
136-
// Must update channel picker before return so that new RPCs will not be routed to deleted
137-
// clusters and resolver can remove them in service config.
138-
updateOverallBalancingState();
139-
140-
shutdownRemoved(acceptRetVal.removedChildren);
141-
} finally {
142-
this.resolvingAddresses = false;
143112
}
144-
145-
return Status.OK;
113+
// Calculate scale
114+
long minWeight = Collections.min(serverWeights.values());
115+
double normalizedMinWeight = (double) minWeight / totalWeight;
116+
// Scale up the number of hashes per host such that the least-weighted host gets a whole
117+
// number of hashes on the the ring. Other hosts might not end up with whole numbers, and
118+
// that's fine (the ring-building algorithm can handle this). This preserves the original
119+
// implementation's behavior: when weights aren't provided, all hosts should get an equal
120+
// number of hashes. In the case where this number exceeds the max_ring_size, it's scaled
121+
// back down to fit.
122+
double scale = Math.min(
123+
Math.ceil(normalizedMinWeight * config.minRingSize) / normalizedMinWeight,
124+
(double) config.maxRingSize);
125+
126+
// Build the ring
127+
ring = buildRing(serverWeights, totalWeight, scale);
128+
129+
return super.acceptResolvedAddresses(resolvedAddresses);
146130
}
147131

148132

‎xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java‎

Lines changed: 7 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -170,31 +170,17 @@ public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
170170
}
171171
config =
172172
(WeightedRoundRobinLoadBalancerConfig) resolvedAddresses.getLoadBalancingPolicyConfig();
173-
AcceptResolvedAddrRetVal acceptRetVal;
174-
try {
175-
resolvingAddresses = true;
176-
acceptRetVal = acceptResolvedAddressesInternal(resolvedAddresses);
177-
if (!acceptRetVal.status.isOk()) {
178-
return acceptRetVal.status;
179-
}
180-
181-
if (weightUpdateTimer != null && weightUpdateTimer.isPending()) {
182-
weightUpdateTimer.cancel();
183-
}
184-
updateWeightTask.run();
185173

186-
createAndApplyOrcaListeners();
174+
if (weightUpdateTimer != null && weightUpdateTimer.isPending()) {
175+
weightUpdateTimer.cancel();
176+
}
177+
updateWeightTask.run();
187178

188-
// Must update channel picker before return so that new RPCs will not be routed to deleted
189-
// clusters and resolver can remove them in service config.
190-
updateOverallBalancingState();
179+
Status status = super.acceptResolvedAddresses(resolvedAddresses);
191180

192-
shutdownRemoved(acceptRetVal.removedChildren);
193-
} finally {
194-
resolvingAddresses = false;
195-
}
181+
createAndApplyOrcaListeners();
196182

197-
return acceptRetVal.status;
183+
return status;
198184
}
199185

200186
/**

0 commit comments

Comments
 (0)