-
Notifications
You must be signed in to change notification settings - Fork 3.9k
api: Expose ForwardingServerBuilder for XdsServerBuilder #7633
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
Conversation
import javax.annotation.Nullable; | ||
|
||
/** | ||
* A {@link ServerBuilder} that delegates all its builder method to another builder by default. |
There was a problem hiding this comment.
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/
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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...
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. |
There was a problem hiding this 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.
442f0c9
to
3ee1b17
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reduces ABI issues caused by returning the more precise
XdsServerBuilder in the API. See
#7552.