Skip to content

Comments

core,netty: expose listening on multiple ports#5306

Merged
carl-mastrangelo merged 8 commits intogrpc:masterfrom
carl-mastrangelo:portyport
Feb 6, 2019
Merged

core,netty: expose listening on multiple ports#5306
carl-mastrangelo merged 8 commits intogrpc:masterfrom
carl-mastrangelo:portyport

Conversation

@carl-mastrangelo
Copy link
Contributor

@carl-mastrangelo carl-mastrangelo commented Feb 1, 2019

Fixes #4134

@carl-mastrangelo carl-mastrangelo added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Feb 4, 2019
@grpc-kokoro grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Feb 4, 2019

/**
* Returns a list of listening sockets for this server. May be different than the originally
* requested sockets (e.g. listening on port '0' maye end up listening on a different port).
Copy link
Contributor

Choose a reason for hiding this comment

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

s/maye/may

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

/**
* Returns a list of listening sockets for this server. May be different than the originally
* requested sockets (e.g. listening on port '0' maye end up listening on a different port).
* Callers should not modify the returned list or the addresses.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe instead say "The returned list is unmodifiable", which also made a requirement for implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

public SocketAddress getListenSocketAddress() {
if (channel == null) {
return -1;
log.fine("server not yet listening on " + address);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this log permanent? It's unusual for a getter to produce logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean. In the case there isn't a listening address (really, bound address), the return value won't be accurate. I can remove it if you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant logs are usually logged when state changes. It's rare for a pure getter that has no side-effect. The only case where the return value is not accurate is when port is 0, which is pretty distinguishable. I would prefer removing 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.

Done.

int port = ts.getPort();
if (port != -1) {
return port;
SocketAddress addr = ts.getListenSocketAddress();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the documentation on Server.getPort() for the case where the server listens on multiple ports, and add a reference to getListenSockets().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added docs on Server.

@carl-mastrangelo carl-mastrangelo merged commit f6ec07d into grpc:master Feb 6, 2019
@carl-mastrangelo carl-mastrangelo deleted the portyport branch February 6, 2019 23:50
@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2019
@carl-mastrangelo carl-mastrangelo restored the portyport branch August 17, 2019 01:11
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.

Support multiple ports in gRPC Java server

3 participants