GH-494: [Flight] Allow configuring connect timeout in JDBC#495
GH-494: [Flight] Allow configuring connect timeout in JDBC#495lidavidm merged 1 commit intoapache:mainfrom
Conversation
|
@jbonofre are you familiar with the JDBC driver at all? |
|
@lidavidm yes a bit. I can take a look on this one. |
jbonofre
left a comment
jbonofre
left a comment
There was a problem hiding this comment.
It looks good to me.
My only comment is that I would expect client cache in another PR as it's a new feature compared to the initial intend of this PR.
| 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.
|
|
||
| <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.
| .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.) |
|
Going to merge since there's been no further comments. |
…che#495) Allow configuring the connect timeout via `connectTimeoutMs` so that the driver doesn't wait so long before giving up on unreachable locations. Fixes apache#494.
2 participants
Allow configuring the connect timeout via
connectTimeoutMsso that the driver doesn't wait so long before giving up on unreachable locations.Fixes #494.