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.

Sign up for GitHub

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

Jump to bottom

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

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

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

wants to merge 55 commits into from

Conversation

SreeramdasLavanya
Copy link
Contributor

@SreeramdasLavanya SreeramdasLavanya commented Oct 17, 2024 •
edited
Loading

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
kannanjgithub
kannanjgithub reviewed Oct 21, 2024
View reviewed changes
api/src/main/java/io/grpc/LoadBalancer.java Outdated
@@ -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
@kannanjgithub kannanjgithub requested a review from ejona86 November 6, 2024 07:18
@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 Nov 6, 2024
shivaspeaks
shivaspeaks reviewed Nov 21, 2024
View reviewed changes
api/src/main/java/io/grpc/LoadBalancer.java
*
* @param resolvedAddresses the resolved server addresses, attributes, and config.
* @since 1.21.0
*/
@Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Add a javadoc on Deprecated and what to use instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Included in Javadoc

ejona86
ejona86 reviewed Nov 21, 2024
View reviewed changes
util/src/main/java/io/grpc/util/GracefulSwitchLoadBalancer.java Show resolved Hide resolved
util/src/main/java/io/grpc/util/MultiChildLoadBalancer.java Outdated
@@ -193,7 +193,7 @@ private List<ChildLbState> updateChildrenWithResolvedAddresses(
newChildLbStates.add(childLbState);
if (entry.getValue() != null) {
childLbState.setResolvedAddresses(entry.getValue()); // update child
childLbState.lb.handleResolvedAddresses(entry.getValue()); // update child LB
childLbState.lb.acceptResolvedAddresses(entry.getValue()); // update child LB
Copy link
Member

Choose a reason for hiding this comment

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

This class needs to fix its handling of the Status. I think we should leave it as-is and create a tracking bug for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving it as-is(handleResolvedAddresses) is resulting in build failure because of the deprecated warning, so i'm retaining this acceptResolvedAddresses for now to proceed further. Please confirm if this is ok and handling the Status can be done as part of a new defect.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, we can definitely suppress warning here and create a tracking bug. Add a comment with TODO mentioning the tracking bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

handled

util/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java Outdated Show resolved Hide resolved
util/src/test/java/io/grpc/util/GracefulSwitchLoadBalancerTest.java Outdated
@@ -125,17 +125,17 @@ public void switchTo_handleResolvedAddressesAndNameResolutionErrorForwardedToLat
helper0.updateBalancingState(READY, picker);

ResolvedAddresses addresses = newFakeAddresses();
gracefulSwitchLb.handleResolvedAddresses(addresses);
verify(lb0).handleResolvedAddresses(addresses);
gracefulSwitchLb.acceptResolvedAddresses(addresses);
Copy link
Member

Choose a reason for hiding this comment

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

Don't change any of these. These shouldn't have caused warnings because they were already marked @Deprecated. They are testing the old code flow and need to remain until it is deleted. When the old flow is deleted, the entire set of "// OLD TESTS" will be outright deleted, as we already have tests below that test acceptResolvedAddresses (and test it more fully, as it checks the return value).

util/src/main/java/io/grpc/util/ForwardingLoadBalancer.java
@@ -30,8 +30,8 @@ public abstract class ForwardingLoadBalancer extends LoadBalancer {
protected abstract LoadBalancer delegate();

@Override
public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) {
Copy link
Member

Choose a reason for hiding this comment

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

This will need some more thought. This change would prevent extending classes from seeing the addresses, if they are only overriding handleResolvedAddresses().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we mark this existing method as deprecated and introduce an override for acceptResolvedAddresses, please confirm

Copy link
Member

Choose a reason for hiding this comment

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

That would still break an implementation only implementing handleResolvedAddresses()

api/src/main/java/io/grpc/LoadBalancer.java Outdated Show resolved Hide resolved
api/src/main/java/io/grpc/LoadBalancer.java Outdated Show resolved Hide resolved
@shivaspeaks shivaspeaks requested a review from ejona86 November 27, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shivaspeaks shivaspeaks shivaspeaks approved these changes

@kannanjgithub kannanjgithub kannanjgithub approved these changes

@ejona86 ejona86 Awaiting requested review from ejona86

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

io.grpc.LoadBalancer method signatures don't match javadoc
5 participants
@SreeramdasLavanya @ejona86 @kannanjgithub @shivaspeaks @grpc-kokoro