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: Expose ForwardingServerBuilder for XdsServerBuilder #7633

Merged
merged 1 commit into from Nov 18, 2020

Conversation

ejona86
Copy link
Member

@ejona86 ejona86 commented Nov 18, 2020

This reduces ABI issues caused by returning the more precise
XdsServerBuilder in the API. See
#7552.

@ejona86 ejona86 added the TODO:release blocker Issue/PR is important enough to delay the release. Removed after release issues resolved label Nov 18, 2020
import javax.annotation.Nullable;

/**
* A {@link ServerBuilder} that delegates all its builder method to another builder by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/all its builder method/all its builder methods/

Copy link
Contributor

@sanjaypujare sanjaypujare left a comment

Choose a reason for hiding this comment

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

LGTM.

/**
* This method serves to force sub classes to "hide" this static factory.
*/
public static ServerBuilder<?> forPort(int port) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The XdsServerBuilder subclass will only define this as VisibleForTesting to be used by test code. Actual user code is supposed to call forPort(int,ServerCredentials) . However the subclass cannot reduce the visibility from public to package-private. Not that we can do anything in this PR about it...

@sergiitk
Copy link
Member

sergiitk commented Nov 18, 2020

Hm, why copy ForwardingServerBuilder and not just make it public? I believe the only reason we made it package-private is because it wasn't used anymore after we reverted NettyServerBuilder to extend AbstractServerImplBuilder.

Copy link
Member

@sergiitk sergiitk left a comment

Choose a reason for hiding this comment

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

I think this is a good reason to make ForwardingServerBuilder public.

This reduces ABI issues caused by returning the more precise
XdsServerBuilder in the API. See
grpc#7552.
@ejona86 ejona86 changed the title xds: Copy ForwardingServerBuilder to xds for XdsServerBuilder api: Expose ForwardingServerBuilder for XdsServerBuilder Nov 18, 2020
@ejona86
Copy link
Member Author

ejona86 commented Nov 18, 2020

I didn't make it public since I didn't want to do that in the 11th hour. But I agree it should be mostly fine. Done.

Copy link
Member

@sergiitk sergiitk left a comment

Choose a reason for hiding this comment

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

:shipit:

@ejona86 ejona86 merged commit 980956d into grpc:master Nov 18, 2020
@ejona86 ejona86 deleted the xds-server-builder-abi branch November 18, 2020 19:31
@ejona86 ejona86 removed the TODO:release blocker Issue/PR is important enough to delay the release. Removed after release issues resolved label Nov 18, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 4, 2021
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.

None yet

3 participants