scwhittle commented on code in PR #34061:
URL: https://github.com/apache/beam/pull/34061#discussion_r1969311575
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/windmill/client/grpc/stubs/WindmillChannelFactory.java:
##########
@@ -58,7 +59,7 @@ public static ManagedChannel remoteChannel(
case GCP_SERVICE_ADDRESS:
return remoteChannel(
windmillServiceAddress.gcpServiceAddress(),
windmillServiceRpcChannelTimeoutSec);
- // switch is exhaustive will never happen.
+ // switch is exhaustive will never happen.
Review Comment:
remove? in the wrong place but don't think it adds much either.
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/windmill/client/grpc/stubs/WindmillChannelFactory.java:
##########
@@ -126,7 +132,7 @@ private WindmillChannelCreationException(HostAndPort
endpoint, SSLException sour
super(
String.format(
"Exception thrown when trying to create channel to
endpoint={host:%s; port:%d}",
- endpoint.getHost(), endpoint.getPort()),
+ endpoint.getHost(), endpoint.hasPort() ? endpoint.getPort() :
-1),
Review Comment:
would be better to show the the port used instead of -1
Can we instead change it so that HostAndPort always has a port set by
construction? Seems like for example we coudl have just factory method
requiring both set or change the parse method to take string+default port if
not in the string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
