-
Notifications
You must be signed in to change notification settings - Fork 4k
autosharding: implementation of EndpointMap #13039
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
base: master
Are you sure you want to change the base?
Changes from all commits
13e6d52
04c6963
d161705
74bdfd1
2488b37
e5b4b22
7dfdf6b
67abc20
789de22
426621f
1b63d20
0771433
1db8017
29957b6
4105fd8
be31260
b99fa0c
12765cf
903ec88
bb9a900
c11754e
c79cc59
c961edf
e350685
ca8fd50
3a3f667
bbf9a6f
f47816f
fa76449
bf6eda3
07b604e
d32c495
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,257 @@ | ||
| /* | ||
| * Copyright 2026 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.autosharding; | ||
|
|
||
| import static com.google.common.base.Preconditions.checkNotNull; | ||
| import static com.google.common.base.Preconditions.checkState; | ||
| import static io.grpc.ConnectivityState.IDLE; | ||
|
|
||
| import com.google.common.base.MoreObjects; | ||
| import com.google.common.collect.ImmutableList; | ||
| import io.grpc.Attributes; | ||
| import io.grpc.ConnectivityState; | ||
| import io.grpc.EquivalentAddressGroup; | ||
| import io.grpc.LoadBalancer; | ||
| import io.grpc.LoadBalancer.FixedResultPicker; | ||
| import io.grpc.LoadBalancer.Helper; | ||
| import io.grpc.LoadBalancer.PickResult; | ||
| import io.grpc.LoadBalancer.ResolvedAddresses; | ||
| import io.grpc.LoadBalancer.SubchannelPicker; | ||
| import io.grpc.util.ForwardingLoadBalancerHelper; | ||
| import io.grpc.util.LazyLoadBalancer; | ||
| import java.util.Collection; | ||
| import java.util.LinkedHashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
| import javax.annotation.Nullable; | ||
| import javax.annotation.concurrent.NotThreadSafe; | ||
|
|
||
| /** | ||
| * Manages the mapping from endpoint hostname to {@link EndpointHolder} and coordinates | ||
| * child load balancer lifecycle and connectivity state updates. | ||
| * | ||
| * <p>Threading model: This class is not thread-safe. All methods must be invoked from the | ||
| * {@link io.grpc.SynchronizationContext} by the parent load balancer. | ||
| */ | ||
| @NotThreadSafe | ||
| final class EndpointMap { | ||
| private final Map<String, EndpointHolder> map = new LinkedHashMap<>(); | ||
|
|
||
| @Nullable | ||
| EndpointHolder get(String hostname) { | ||
|
Contributor
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. I believe we should to add javadocs for all package private methods, assuming they are package private because they need to be used by other classes in this package i.e. they represent an API surface. If not, let's make them private. |
||
| return map.get(checkNotNull(hostname, "hostname")); | ||
| } | ||
|
|
||
| void put(String hostname, EndpointHolder holder) { | ||
| map.put(checkNotNull(hostname, "hostname"), checkNotNull(holder, "holder")); | ||
|
Contributor
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. The overuse of |
||
| } | ||
|
|
||
| @Nullable | ||
| EndpointHolder remove(String hostname) { | ||
| return map.remove(checkNotNull(hostname, "hostname")); | ||
| } | ||
|
|
||
| Collection<EndpointHolder> values() { | ||
| return map.values(); | ||
| } | ||
|
|
||
| Set<String> keySet() { | ||
| return map.keySet(); | ||
| } | ||
|
|
||
| int size() { | ||
| return map.size(); | ||
| } | ||
|
|
||
| boolean isEmpty() { | ||
| return map.isEmpty(); | ||
| } | ||
|
|
||
| void clear() { | ||
| map.clear(); | ||
| } | ||
|
|
||
| /** | ||
| * Re-assigns contiguous 0-based index values across all current endpoint holders. | ||
| */ | ||
| void reindex() { | ||
|
Contributor
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. Why do we need this method? Is the user expected to call this before converting to PickerEndpoints? Even if that's the case, if caller removes things from the map and then calls We need to really think more about defining the overall abstraction for EndpointMap. |
||
| int nextIdx = 0; | ||
| for (EndpointHolder holder : map.values()) { | ||
| holder.setIndex(nextIdx++); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Shuts down all child load balancers and clears the map. | ||
| */ | ||
| void shutdownAll() { | ||
|
Contributor
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. Why do we need both shutdownAll and clear ? |
||
| for (EndpointHolder holder : map.values()) { | ||
| holder.shutdown(); | ||
|
Contributor
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. In the control plane operation we shutdown map , which shuts holder , which shuts childLb. But threre could be a race between control plane shutdown and the data plane |
||
| } | ||
| map.clear(); | ||
| } | ||
|
|
||
| /** | ||
| * Builds an immutable snapshot list of {@link PickerEndpoint}s placed strictly at their | ||
| * corresponding {@link EndpointHolder#getIndex()} positions. | ||
| * | ||
| * @throws IllegalStateException if endpoint indices are not contiguous from 0 to N-1 | ||
| */ | ||
| ImmutableList<PickerEndpoint> toPickerEndpoints() { | ||
| int size = map.size(); | ||
| if (size == 0) { | ||
| return ImmutableList.of(); | ||
| } | ||
| PickerEndpoint[] array = new PickerEndpoint[size]; | ||
| for (EndpointHolder holder : map.values()) { | ||
| int idx = holder.getIndex(); | ||
| checkState( | ||
|
Contributor
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. So, this is what my previous comment was talking about. The class is currently very easy to use incorrectly and trigger these exceptions. GIven that this runs in the synchronization context, this could break quite a lot of things . |
||
| idx >= 0 && idx < size, | ||
| "Endpoint holder index %s is out of bounds for size %s", | ||
| idx, | ||
| size); | ||
| checkState( | ||
| array[idx] == null, | ||
| "Duplicate endpoint holder index %s detected", | ||
| idx); | ||
| array[idx] = holder.toPickerEndpoint(); | ||
| } | ||
| return ImmutableList.copyOf(array); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return MoreObjects.toStringHelper(this) | ||
| .add("map", map) | ||
| .toString(); | ||
| } | ||
|
|
||
| /** | ||
| * Holds the connectivity state, picker, and lazy child load balancer for a single endpoint. | ||
| */ | ||
| static final class EndpointHolder { | ||
| private int index; | ||
|
Contributor
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 discuss more about the need for index. |
||
| private final LazyLoadBalancer childLb; | ||
| private final AtomicBoolean connectingScheduled = new AtomicBoolean(false); | ||
|
Contributor
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. Why AtomicBoolean? Is this class supposed to be threadsafe? Seems counterintuitive if it's supposed to be held in a class that's not threadsafe, unless we expect people to get it from the map which may not be the correct abstraction for EndpointHolder. |
||
| private final Helper helper; | ||
| private ConnectivityState state = IDLE; | ||
| private SubchannelPicker picker = new FixedResultPicker(PickResult.withNoResult()); | ||
|
|
||
| EndpointHolder( | ||
| int index, | ||
| Helper helper, | ||
| LoadBalancer.Factory pickFirstFactory, | ||
| @Nullable Runnable stateUpdateCallback) { | ||
|
Contributor
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. Anything that we want to document here about what we expect from the callback? |
||
| this.index = index; | ||
| this.helper = checkNotNull(helper, "helper"); | ||
| this.childLb = new LazyLoadBalancer( | ||
| new ChildHelper(helper, stateUpdateCallback), | ||
| checkNotNull(pickFirstFactory, "pickFirstFactory")); | ||
|
Contributor
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. nit: We may want to move |
||
| } | ||
|
|
||
| int getIndex() { | ||
| return index; | ||
| } | ||
|
|
||
| void setIndex(int index) { | ||
| this.index = index; | ||
| } | ||
|
|
||
| ConnectivityState getState() { | ||
|
Contributor
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. Since I don't know how it's supposed to be used, I can't say much, but it seems like we've created a getter for each and every fields, making this field like a struct rather than class that needs to enforce invariants. Do we need everything here? |
||
| return state; | ||
| } | ||
|
|
||
| SubchannelPicker getPicker() { | ||
| return picker; | ||
| } | ||
|
|
||
| LazyLoadBalancer getChildLb() { | ||
| return childLb; | ||
| } | ||
|
|
||
| PickerEndpoint toPickerEndpoint() { | ||
| return new PickerEndpoint(state, picker, this::exitIdle); | ||
| } | ||
|
|
||
| private void exitIdle() { | ||
| if (connectingScheduled.compareAndSet(false, true)) { | ||
| helper.getSynchronizationContext().execute(() -> { | ||
| connectingScheduled.set(false); | ||
|
Contributor
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. So, we move atomic from false to true, then we execute stuff on synccontext , but then we set it to false again before requesting connection. So, this means while we are requesting connection, other rpcs can again trigger requesting connection? What are we tryting to achieve here? This seems to be doing nothing. Shouldn't this be set to false only when the connection becomes idle again instead of before we start requesting connection? |
||
| childLb.requestConnection(); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| void updateAddresses(List<EquivalentAddressGroup> eags, Attributes attributes) { | ||
| ResolvedAddresses childAddresses = ResolvedAddresses.newBuilder() | ||
| .setAddresses(ImmutableList.copyOf(checkNotNull(eags, "eags"))) | ||
| .setAttributes(checkNotNull(attributes, "attributes")) | ||
| .build(); | ||
| childLb.acceptResolvedAddresses(childAddresses); | ||
|
Contributor
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. Seems like we are swallowing errors from childLb here, intentional? |
||
| } | ||
|
|
||
| void requestConnection() { | ||
|
Contributor
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. Why do we need this if we already have exitIdle? This seems like a very unsafe verision which doesn't bother executing on sync context and doesn't check the atomic boolean. |
||
| childLb.requestConnection(); | ||
| } | ||
|
|
||
| void shutdown() { | ||
| childLb.shutdown(); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return MoreObjects.toStringHelper(this) | ||
| .add("index", index) | ||
| .add("state", state) | ||
| .add("childLb", childLb) | ||
| .toString(); | ||
| } | ||
|
|
||
| private final class ChildHelper extends ForwardingLoadBalancerHelper { | ||
| private final Helper delegateHelper; | ||
| @Nullable private final Runnable stateUpdateCallback; | ||
|
|
||
| ChildHelper(Helper delegateHelper, @Nullable Runnable stateUpdateCallback) { | ||
| this.delegateHelper = checkNotNull(delegateHelper, "delegateHelper"); | ||
| this.stateUpdateCallback = stateUpdateCallback; | ||
| } | ||
|
|
||
| @Override | ||
| protected Helper delegate() { | ||
| return delegateHelper; | ||
| } | ||
|
|
||
| @Override | ||
| public void updateBalancingState(ConnectivityState newState, SubchannelPicker newPicker) { | ||
| state = checkNotNull(newState, "newState"); | ||
| picker = checkNotNull(newPicker, "newPicker"); | ||
| if (stateUpdateCallback != null) { | ||
| stateUpdateCallback.run(); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return MoreObjects.toStringHelper(this) | ||
| .add("delegateHelper", delegateHelper) | ||
| .toString(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
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.
variable name seems too generic.