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

Mark MultiChildLoadBalancer as Internal. #10481

Merged
merged 5 commits into from
Aug 16, 2023

Conversation

larry-safran
Copy link
Contributor

Cannot move to the intern package because of its use of classes in the util package.
There is precedence for using the @internal annotation as it is being done by outlier detection and round robin.

…package because of its use of classes in the util package.
@larry-safran larry-safran requested a review from ejona86 August 14, 2023 22:57
@ejona86
Copy link
Member

ejona86 commented Aug 14, 2023

Cannot move to the intern package because of its use of classes in the util package.

Yeah, ForwardingLoadBalancerHelper should really be in io.grpc.

There is precedence for using the @internal annotation as it is being done by outlier detection and round robin.

They are essentially doing it wrong as well. The only @Internal things should have internal in their name, or be Providers. We do that for clarity, and also because we have Blaze globs internally that prevent users from using our internal APIs without permission (because people kept using them, and then surprised when it broke because the person that did it isn't on the team any more).

RoundRobinLoadBalancer is public as a hack to share code with WRR, which had to be implemented very quickly. There should be some code sharing, but we haven't gotten that nice yet (and this base class might actually replace what was done there).

OutlierDetectionLoadBalancer looks to be public because the config is used from xds. That should just use json and the Provider.

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

For the super short-term (like this coming release), we can accept this, but let's exclude it from the Javadoc as well. Add to util/build.gradle (and confirm it isn't present in the javadoc):

tasks.named("javadoc").configure {
    exclude 'io/grpc/util/MultiChildLoadBalancer.java'
}

@@ -186,3 +186,7 @@ publishing {
}
}
}

tasks.named("javadoc").configure {
Copy link
Member

Choose a reason for hiding this comment

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

Remove this? I didn't think this was needed.

@larry-safran larry-safran merged commit f906562 into grpc:master Aug 16, 2023
@larry-safran larry-safran deleted the moveMultiChild branch August 16, 2023 00:33
larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Aug 29, 2023
* Mark MultiChildLoadBalancer as Internal.  Cannot move to the internal package because of its use of classes in the util package.

* Exclude MultiChildLoadBalancer from javadoc generation.

* Fix javadoc creation.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants