jbonofre commented on code in PR #1282:
URL: https://github.com/apache/arrow-java/pull/1282#discussion_r3905110601
##########
flight/flight-core/src/main/java/org/apache/arrow/flight/grpc/NettyClientBuilder.java:
##########
@@ -140,9 +140,9 @@ public NettyChannelBuilder build() {
case LocationSchemes.GRPC_TLS:
{
final int port = location.getUri().getPort();
- if (port < 0 || port > 65535) {
+ if (port < 1 || port > 65535) {
throw new IllegalArgumentException(
- "Invalid port " + port + ": must be between 0 and 65535.");
+ "Invalid port " + port + ": must be between 1 and 65535.");
Review Comment:
This narrows the accepted input range of published public API:
`NettyClientBuilder`,
and transitively `FlightClient.Builder`. Third-party code that today calls
`FlightClient.builder(allocator, someLocation).build()` with a port-0 location
(a server location captured before `start()`, or a placeholder resolved
later) gets a lazily-failing channel today and an immediate
`IllegalArgumentException` after this.
The change itself is right, but neither `NettyClientBuilder.build()` nor
`FlightClient.Builder.build()` documents the precondition.
Could we add an `@throws IllegalArgumentException` javadoc stating the
accepted range, so callers can
discover it without hitting it in production?
##########
flight/flight-core/src/main/java/org/apache/arrow/flight/grpc/NettyClientBuilder.java:
##########
@@ -140,9 +140,9 @@ public NettyChannelBuilder build() {
case LocationSchemes.GRPC_TLS:
{
final int port = location.getUri().getPort();
- if (port < 0 || port > 65535) {
+ if (port < 1 || port > 65535) {
Review Comment:
`URI.getPort()` returns `-1` to mean "no port present in the URI", not "the
user asked for port -1".
Folding it into this range check produces `Invalid port -1: must be between
1 and 65535` (a number that the user never defined).
That path is reachable: a `Location` like `grpc+tls://myhost` with no port,
and also `ArrowFlightSqlClientHandler.getStreams()`, which does
`withPort(endpointUri.getPort())` on an endpoint URI that may carry no port.
Since this PR is rewriting the message anyway, this seems like the moment to
split
the two cases:
```java
if (port == -1) {
throw new IllegalArgumentException(
"No port specified in location URI: " + location.getUri());
}
if (port < 1 || port > 65535) {
throw new IllegalArgumentException(
"Invalid port " + port + ": must be between 1 and 65535.");
}
```
--
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]
