Skip to content
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

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

Open
jdcormie opened this issue May 9, 2024 · 4 comments · May be fixed by #11623
Open

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

jdcormie opened this issue May 9, 2024 · 4 comments · May be fixed by #11623
Labels
Milestone

Comments

@jdcormie
Copy link
Member

jdcormie commented May 9, 2024

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.

@ejona86 ejona86 added this to the Next milestone May 10, 2024
@ejona86 ejona86 added the docs label May 10, 2024
@kannanjgithub
Copy link
Contributor

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.

@ejona86
Copy link
Member

ejona86 commented Oct 21, 2024

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.

@SreeramdasLavanya
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants