GH-494: [Flight] Allow configuring connect timeout in JDBC #495
Conversation
|
@jbonofre are you familiar with the JDBC driver at all? |
|
@lidavidm yes a bit. I can take a look on this one. |
| public NettyChannelBuilder build() { | ||
| final NettyChannelBuilder builder; | ||
|
|
||
| switch (location.getUri().getScheme()) { |
There was a problem hiding this comment.
FYI, I'm investigating the Netty channel address depending of the scheme as it seems we have a weird behavior here.
Your change move that from FlightClient to NettyClientBuilder so it looks good to me.
| @@ -47,6 +47,21 @@ under the License. | |||
| </exclusions> | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
| <groupId>io.grpc</groupId> | |||
| <artifactId>grpc-api</artifactId> | |||
There was a problem hiding this comment.
I'm surprised we have to explicitly define these dependencies, I thought they came transitively from arrow flight.
| @@ -113,6 +114,8 @@ private static ArrowFlightSqlClientHandler createNewClientHandler( | |||
| .withRetainCookies(config.retainCookies()) | |||
| .withRetainAuth(config.retainAuth()) | |||
| .withCatalog(config.getCatalog()) | |||
| .withClientCache(config.useClientCache() ? new FlightClientCache() : null) | |||
There was a problem hiding this comment.
That's a new feature (client caching), right ? It's ok in the PR but it looks a big "unrelated" to the PR title 😄
|
Thanks, I can split up this PR into 3? parts if that would be better. |
This comment has been minimized.
This comment has been minimized.
|
@lidavidm no, that's OK. I was just a bit confused because the change seems larger than just the PR title. But it's OK, I'm fine with that. I will do a new review pass. Thanks ! |
|
It's probably better for the changelog if I split it up so no worries! Thanks! |
lidavidm
changed the title
|
Ok, I've cut this down to just the connect timeout option. I'll follow up with a third PR to add back the basic client cache. (There's yet another issue for adding a full client cache.) |
Allow configuring the connect timeout via
connectTimeoutMsso that the driver doesn't wait so long before giving up on unreachable locations.Fixes #494.