autosharding: implementation of EndpointMap - #13039
shivaspeaks wants to merge 32 commits into
Conversation
…harding-part2-picker
…harding-part3-lazy-endpoints
…harding-part3-lazy-endpoints
shivaspeaks
changed the title
shivaspeaks
changed the title
sauravzg
left a comment
sauravzg
left a comment
There was a problem hiding this comment.
Reviewed the sources.
This PR in its current state is very difficult to review. We have classes with a lot of getters, setters, invariants and state manipulation some of which lead to inconsistent state.
We should either couple this PR with the class that uses it so that we can understand the expecations from the class, or meticulously document the expectations as our contract in this class to make the review easier.
Happy to discuss offline if needed.
| static final class EndpointHolder { | ||
| private int index; | ||
| private final LazyLoadBalancer childLb; | ||
| private final AtomicBoolean connectingScheduled = new AtomicBoolean(false); |
There was a problem hiding this comment.
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.
| */ | ||
| @NotThreadSafe | ||
| final class EndpointMap { | ||
| private final Map<String, EndpointHolder> map = new LinkedHashMap<>(); |
There was a problem hiding this comment.
variable name seems too generic.
| /** | ||
| * Re-assigns contiguous 0-based index values across all current endpoint holders. | ||
| */ | ||
| void reindex() { |
There was a problem hiding this comment.
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 get , won't the caller end up wih endpoint state with invalid indices?
We need to really think more about defining the overall abstraction for EndpointMap.
If we need invariants, it cannot trivially provide all map functionalities unless we plan to call reindex everytim at the cost of performance.
| } | ||
|
|
||
| void put(String hostname, EndpointHolder holder) { | ||
| map.put(checkNotNull(hostname, "hostname"), checkNotNull(holder, "holder")); |
There was a problem hiding this comment.
The overuse of checknotNull throughout the PR makes it very difficult to read.
go/java-practices/null#tolerant
| PickerEndpoint[] array = new PickerEndpoint[size]; | ||
| for (EndpointHolder holder : map.values()) { | ||
| int idx = holder.getIndex(); | ||
| checkState( |
There was a problem hiding this comment.
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 .
| private void exitIdle() { | ||
| if (connectingScheduled.compareAndSet(false, true)) { | ||
| helper.getSynchronizationContext().execute(() -> { | ||
| connectingScheduled.set(false); |
There was a problem hiding this comment.
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.acceptResolvedAddresses(childAddresses); | ||
| } | ||
|
|
||
| void requestConnection() { |
There was a problem hiding this comment.
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.
| int index, | ||
| Helper helper, | ||
| LoadBalancer.Factory pickFirstFactory, | ||
| @Nullable Runnable stateUpdateCallback) { |
There was a problem hiding this comment.
Anything that we want to document here about what we expect from the callback?
| private final Map<String, EndpointHolder> map = new LinkedHashMap<>(); | ||
|
|
||
| @Nullable | ||
| EndpointHolder get(String hostname) { |
There was a problem hiding this comment.
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.
| .setAddresses(ImmutableList.copyOf(checkNotNull(eags, "eags"))) | ||
| .setAttributes(checkNotNull(attributes, "attributes")) | ||
| .build(); | ||
| childLb.acceptResolvedAddresses(childAddresses); |
There was a problem hiding this comment.
Seems like we are swallowing errors from childLb here, intentional?
3 participants
This is Part3 of gRFC A119, introducing EndpointMap.
This is responsible for managing individual backend child load balancers lazily, keeping uncontacted endpoints in IDLE until an RPC is assigned to them. We are calling
EndpointStateasEndpointHolderin this implementation to avoid name collision with actual endpointState coming fromcom.google.cloud.autosharding.v1.EndpointState.