autosharding: implementation of EndpointMap, autosharding client and LB Policy

[shivaspeaks]

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 EndpointState as EndpointHolder in this implementation to avoid name collision with actual endpointState coming from com.google.cloud.autosharding.v1.EndpointState.

[sauravzg]

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.

[sauravzg]

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.

[shivaspeaks]

Oh right! This class is indeed @notThreadSafe and confined to the synchronization context. The only entry point from RPC threads is PickerEndpoint.requestConnection, which hops onto the sync context before touching anything.

[sauravzg]

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?

[shivaspeaks]

Oh right! This class is indeed @notThreadSafe and confined to the synchronization context. The only entry point from RPC threads is PickerEndpoint.requestConnection, which hops onto the sync context before touching anything.

[sauravzg]

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.

[shivaspeaks]

Fixed with a childShutdown flag on the holder that is checked inside the synchronization context, after the thread hop, so it cannot be stale by the time it is read.

Is our PickerEndpoint truly immutable?

Yes, PickerEndpoint is immutable. It captures the state and picker by value, and its only reference back into this class is the ExitIdler, which does nothing but schedule work on the synchronization context.

[shivaspeaks]

I've added the LB policy that consumes it (AutoShardingLoadBalancer) and the AutoshardingClient it depends on, so every expectation on EndpointMap is now visible at a call site.

Note for updates in EndpointMap:

1. Index is now list position, not a stored field.
2. Index assigned after de-duplication.
3. Rebuild in place instead of a fresh map: every grpc-java LB retains children by key. This is what makes the rebuilding flag necessary: LazyLoadBalancer publishes IDLE synchronously from acceptResolvedAddresses, so without it the LB would build a picker from a half-rebuilt map.
4. The picker snapshot is built here, not in the picker.
5. requestConnection returns early unless IDLE, where the gRFC calls ExitIdle() unconditionally.
6. AutoShardingAttributes.ATTR_ENDPOINT_HOSTNAME and XdsInternalAttributes.ATTR_ADDRESS_NAME (gRFC A81) carry the same value but are two different keys. We need to bridge them when we do xds integration or we can refactor it out in util, we simply cannot let autosharding have a dependency on xds because then it will be cycle when we do xds integration.

[shivaspeaks]

I will re-request for your review after I do my final review. Please wait for the re-request @sauravzg before taking a look.