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

api: Javadoc changes for io.grpc.LoadBalancer method signatures #11623

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

SreeramdasLavanya
Copy link
Contributor

@SreeramdasLavanya SreeramdasLavanya commented Oct 17, 2024

Javadoc changes for io.grpc.LoadBalancer

Fixes #11194

@SreeramdasLavanya
Copy link
Contributor Author

Below points have been addressed:

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

@SreeramdasLavanya SreeramdasLavanya marked this pull request as ready for review October 18, 2024 12:40
@@ -179,12 +179,12 @@ public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) {
* EquivalentAddressGroup} addresses should be considered equivalent but may be flattened into a
* single list if needed.
*
* <p>Implementations can choose to reject the given addresses by returning {@code false}.
* <p>Implementations can choose to reject the given addresses by returning {@code UNAVAILABLE}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace with {@code Status.UNAVAILABLE} . Also below.

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 22, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 22, 2024
@@ -263,7 +263,7 @@ public void shutdown() {
new EquivalentAddressGroup(new SocketAddress(){}));
ResolvedAddresses addresses = ResolvedAddresses.newBuilder().setAddresses(servers)
.setAttributes(attrs).build();
balancer.handleResolvedAddresses(addresses);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed not to change this test.

@@ -197,7 +201,7 @@ public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
return unavailableStatus;
} else {
if (recursionCount++ == 0) {
handleResolvedAddresses(resolvedAddresses);
//handleResolvedAddresses(resolvedAddresses);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have you commented this out?

// parent policies, where they fail to call switchTo(). So we will use the exception to try
// to notice those bugs quickly, as it will fail very loudly.
throw new IllegalStateException(
"GracefulSwitchLoadBalancer must switch to a load balancing policy before handling"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must be added instead to acceptResolvedAddresses.

@@ -240,9 +240,8 @@ public void handleResolvedAddresses_delegatesToAcceptResolvedAddresses() {

LoadBalancer balancer = new LoadBalancer() {
@Override
public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) {
resultCapture.set(resolvedAddresses);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? Revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rolled back please review

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

Successfully merging this pull request may close these issues.

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