grpc-java icon indicating copy to clipboard operation
grpc-java copied to clipboard

io.grpc.LoadBalancer method signatures don't match javadoc

Open jdcormie opened this issue 1 year ago • 5 comments

javadoc for public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses)

  • refers to non-existent argument servers

javadoc for public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses)

  • refers to an argument named addresses
  • refers to returning values not of the actual return type

Readers are probably also curious about the difference between "accept" and "handle" and which one implementations are expected to override.

jdcormie avatar May 09 '24 21:05 jdcormie

Readers are probably also curious about the difference between "accept" and "handle" and which one implementations are expected to override.

I think acceptResolvedAddresses is the one implementations should prefer to use because it returns a status of the acceptance back to the name resolution system. However handleResolvedAddresses is internally still used in the multi level Xds load balancers, which is probably why it is not deprecated. @ejona86 please confirm.

kannanjgithub avatar Oct 21 '24 08:10 kannanjgithub

handleResolvedAddresses is internally still used in the multi level Xds load balancers

That should be updated to call the new method.

why it is not deprecated

It isn't deprecated because we needed to make the StatusOr<addresses> change to the LB API and it wasn't clear at the time whether that would be API compatible. We didn't want people to move over to acceptResolvedAddresses() only for us to then create a acceptResolvedAddresses2() for the addresses part. At this point, it seems fine for it to be deprecated.

ejona86 avatar Oct 21 '24 13:10 ejona86

handleResolvedAddresses is internally still used in the multi level Xds load balancers

That should be updated to call the new method.

why it is not deprecated

It isn't deprecated because we needed to make the StatusOr<addresses> change to the LB API and it wasn't clear at the time whether that would be API compatible. We didn't want people to move over to acceptResolvedAddresses() only for us to then create a acceptResolvedAddresses2() for the addresses part. At this point, it seems fine for it to be deprecated.

@kannanjgithub @ejona86

As per our understanding I'm marking handleResolvedAddresses method as deprecated and updating the references(14 usages of handleResolvedAddresses method).

SreeramdasLavanya avatar Oct 22 '24 09:10 SreeramdasLavanya

@ejona86 @kannanjgithub

While marking handleResolvedAddresses method as deprecated we observed that following cyclic calls in LoadBalancer as follows I.e., handleResolvedAddresses -> calls acceptResolvedAddresses -> calls handleResolvedAddresses which we are trying to mark as deprecated. Also not sure about significance of recursionCount, at this point require suggestions it would be helpful to proceed further.

SreeramdasLavanya avatar Oct 25 '24 10:10 SreeramdasLavanya