Skip to content
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

util: Store only a list of children in MultiChildLB #11578

Merged
merged 2 commits into from
Oct 2, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 32 additions & 29 deletions util/src/main/java/io/grpc/util/MultiChildLoadBalancer.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
import static io.grpc.ConnectivityState.TRANSIENT_FAILURE;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.base.Objects;
import com.google.common.collect.Maps;
import io.grpc.Attributes;
import io.grpc.ConnectivityState;
import io.grpc.EquivalentAddressGroup;
Expand All @@ -37,12 +38,9 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.Nullable;
Expand All @@ -55,7 +53,8 @@
public abstract class MultiChildLoadBalancer extends LoadBalancer {

private static final Logger logger = Logger.getLogger(MultiChildLoadBalancer.class.getName());
private final Map<Object, ChildLbState> childLbStates = new LinkedHashMap<>();
// Modify by replacing the list to release memory when no longer used.
private List<ChildLbState> childLbStates = new ArrayList<>(0);
private final Helper helper;
// Set to true if currently in the process of handling resolved addresses.
protected boolean resolvingAddresses;
Expand Down Expand Up @@ -84,7 +83,8 @@
*/
protected Map<Object, ResolvedAddresses> createChildAddressesMap(
ResolvedAddresses resolvedAddresses) {
Map<Object, ResolvedAddresses> childAddresses = new HashMap<>();
Map<Object, ResolvedAddresses> childAddresses =
Maps.newLinkedHashMapWithExpectedSize(resolvedAddresses.getAddresses().size());
for (EquivalentAddressGroup eag : resolvedAddresses.getAddresses()) {
ResolvedAddresses addresses = resolvedAddresses.toBuilder()
.setAddresses(Collections.singletonList(eag))
Expand Down Expand Up @@ -144,7 +144,7 @@
@Override
public void shutdown() {
logger.log(Level.FINE, "Shutdown");
for (ChildLbState state : childLbStates.values()) {
for (ChildLbState state : childLbStates) {
state.shutdown();
}
childLbStates.clear();
Expand All @@ -169,39 +169,37 @@
return new AcceptResolvedAddrRetVal(unavailableStatus, null);
}

updateChildrenWithResolvedAddresses(newChildAddresses);

return new AcceptResolvedAddrRetVal(Status.OK, getRemovedChildren(newChildAddresses.keySet()));
List<ChildLbState> removed = updateChildrenWithResolvedAddresses(newChildAddresses);
return new AcceptResolvedAddrRetVal(Status.OK, removed);
}

private void updateChildrenWithResolvedAddresses(
/** Returns removed children. */
private List<ChildLbState> updateChildrenWithResolvedAddresses(
Map<Object, ResolvedAddresses> newChildAddresses) {
// Create a map with the old values
Map<Object, ChildLbState> oldStatesMap =
Maps.newLinkedHashMapWithExpectedSize(childLbStates.size());
for (ChildLbState state : childLbStates) {
oldStatesMap.put(state.getKey(), state);
}

// Move ChildLbStates from the map to a new list (preserving the new map's order)
List<ChildLbState> newChildLbStates = new ArrayList<>(newChildAddresses.size());
for (Map.Entry<Object, ResolvedAddresses> entry : newChildAddresses.entrySet()) {
ChildLbState childLbState = childLbStates.get(entry.getKey());
ChildLbState childLbState = oldStatesMap.remove(entry.getKey());
if (childLbState == null) {
childLbState = createChildLbState(entry.getKey());
childLbStates.put(entry.getKey(), childLbState);
}
newChildLbStates.add(childLbState);
if (entry.getValue() != null) {
childLbState.setResolvedAddresses(entry.getValue()); // update child
childLbState.lb.handleResolvedAddresses(entry.getValue()); // update child LB
}
}
}

/**
* Identifies which children have been removed (are not part of the newChildKeys).
*/
private List<ChildLbState> getRemovedChildren(Set<Object> newChildKeys) {
List<ChildLbState> removedChildren = new ArrayList<>();
// Do removals
for (Object key : ImmutableList.copyOf(childLbStates.keySet())) {
if (!newChildKeys.contains(key)) {
ChildLbState childLbState = childLbStates.remove(key);
removedChildren.add(childLbState);
}
}
return removedChildren;
childLbStates = newChildLbStates;
// Remaining entries in map are orphaned
return new ArrayList<>(oldStatesMap.values());
}

protected final void shutdownRemoved(List<ChildLbState> removedChildren) {
Expand Down Expand Up @@ -236,12 +234,17 @@

@VisibleForTesting
public final Collection<ChildLbState> getChildLbStates() {
return childLbStates.values();
return childLbStates;
}

@VisibleForTesting
public final ChildLbState getChildLbState(Object key) {
return childLbStates.get(key);
for (ChildLbState state : childLbStates) {
larry-safran marked this conversation as resolved.
Show resolved Hide resolved
if (Objects.equal(state.getKey(), key)) {
return state;
}
}
return null;

Check warning on line 247 in util/src/main/java/io/grpc/util/MultiChildLoadBalancer.java

View check run for this annotation

Codecov / codecov/patch

util/src/main/java/io/grpc/util/MultiChildLoadBalancer.java#L247

Added line #L247 was not covered by tests
}

@VisibleForTesting
Expand Down
Loading