[shivaspeaks]

gRFC A119

[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.

[sauravzg]

Reviewed all of source. Some comments may be outdated since this was a large PR and I have been incrementally reviewing it for quite some time.

[sauravzg]

nit: go/java-practices/constants#when-to-use-a-constant-versus-an-inlined-literal

[sauravzg]

Why is proto3 is relevant here if our input is a map? IIRC, proto3 when converted to json has a consistent behavior that the key is ommitted.

For us , I don't think the distinction even matters. Empty and missing are both garbage for us.

[sauravzg]

The grfc should actively call out valid or invalid configurations.

We should fail as early as possible. An empty address will never connect and the config should be rejected , there's no point passing it along to the channel. I assume the same for channelFactoryKey.

Go seems to treat all of them as mandatory: https://github.com/grpc/grpc-go/blob/5ff04b66ea52b067fab16c735f330f6c2528c53a/balancer/autosharding/config.go#L46-L55

[sauravzg]

Would this benefit from autovalue ?

[sauravzg]

The docs are leaky. Docs should talk about the contract of an entity i.e. how the entity is supposed to be used , not leak how the implmentation actually uses it.

[sauravzg]

Merge it with >0 , with a single error message.

[sauravzg]

similar to above, sorted , dropped , kept are very generic variable names.

[sauravzg]

this'll throw if runEndKey is null, which'll happen if the first entry is [x, inf) and the second entry is finite. Remember to add tests for the fix.

[sauravzg]

Nit: I understand that this follows precendent from other implementations like LrsStream etc , where we have stream handle all of the lifecycle and handle call as well as observing the responses.

I'd like you to explore composition here to break down responsibilities a bit where we separate the call and callobserver side of things. I haven't thought about it but this is what I'd come up with on an initial glance.

private final class AutoshardingStream {
  private final List<AssignmentChunk> bufferedChunks = new ArrayList<>();
  private ClientCallStreamObserver<WatchShardingAssignmentRequest> requestStream;
  private boolean receivedGoodAssignment;
  private boolean closed;
  void start() {
     // start the call using ResponseObserver
  }
  void close(Status status) { ... requestStream.cancel(...); }
  private final class ResponseObserver
      implements ClientResponseObserver<WatchShardingAssignmentRequest,
          WatchShardingAssignmentResponse> {
    @Override public void beforeStart(ClientCallStreamObserver<WatchShardingAssignmentRequest> rs) {
      requestStream = rs;
    }
    @Override public void onNext(WatchShardingAssignmentResponse r) {
      syncContext.execute(() -> handleResponse(r));
    }
    @Override public void onError(Throwable t) {
      syncContext.execute(() -> handleStreamClosed(Status.fromThrowable(t)));
    }
    @Override public void onCompleted() { ... }
  }
}

[sauravzg]

Why are we using the generic API instead of intantiating a stub and calling the method on the stub?