GH-577: Use JVM proxy settings in Netty client - #1027
Conversation
This comment has been minimized.
This comment has been minimized.
|
As I'm close to release Arrow Java 19.0.0, I will consider this PR for 20.0.0. |
|
@jbonofre yeah that makes sense, this one is still a draft as I have not finished work on it, I will ping you once I finish the PR. Should be ready in the next couple of days. |
ef9e0c5 to
1e44df0
Compare
|
While the fix was in the Let me know if it's worth it to expand the testing surface with a unit test closer to the place of the bug fix. |
|
Can we rebase? It seems CI did not trigger |
1e44df0 to
dcfa5ef
Compare
|
It seems all workflows time out here, even after a retry. |
|
@lidavidm Thanks for the ping. I rebased all PRs. |
Do you guys need help with the CI? I was going to look into some CI stuff on Arrow Go, but given that we have a release here I can jump in before I move to that. Edit- I synced with JB and I saw that he is already on it. I'll continue with the protocol change implementation in other languages to have feature parity. |
|
Well, other PRs are passing fine. Perhaps something in this PR may be related? |
dcfa5ef to
18b30dd
Compare
|
I tracked this down all the way to grpc-java and IMO the root cause is there, my change in this PR surfaced the problem. The situation looks a bit involved. I can create some PRs in Arrow Java to gain some robustness over these lower level errors in time for this release, or we can leave this PR for a later release. I have no strong opinion either way, but I do have bandwidth to tackle at least one of these right now. If you are interested @lidavidm and @jbonofre let me know and I'll start work on this. Improvements we could have in the JDBC driver:
Details about the root cause (no need to read this unless you're interested in the gRPC internals): 1- I changed the NettyChannelBuilder call from forAddress(SocketAddress) to forAddress(String, int). This allows us to respect JVM proxy settings and is the overload gRPC recommends for normal TCP channels. I will take this up with the gRPC folks and see if they agree that we need to propagate the exception there. In any case that will be too slow for this release. |
|
Thanks for digging into it. I think we should do the validation here, and implement the timeout and follow up with grpc-java separately. |
|
@lidavidm I'll add the port validation in this PR later today. I think that makes sense as it both unblocks the PR and it is my change here that removed the fail-fast validation. I created a couple issues for the JDBC connection timeouts: #1267 #1268 I'll grab them once I finish here, I already familiarized myself with the relevant code yesterday. I think it's fine to leave them for a later release though. I'll also follow-up with gRPC (and try to provide a PR there if they agree that the problem is on their side). Thanks for the guidance! Edit- I will finish this PR tomorrow, as I had a small health hiccup and can't look at screens for a long time for the rest of today. |
….getPort() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Validate the port synchronously in NettyClientBuilder.build() before calling forAddress(String, int), restoring the fail-fast behaviour that was lost when switching away from forAddress(SocketAddress). Placing the check here protects all callers (not just JDBC), and the resulting IllegalArgumentException is already caught and wrapped as SQLException by ArrowFlightSqlClientHandler.Builder.build(). Reverts the validation that was previously added to ArrowFlightConnectionConfigImpl.getPort() and the associated throws-SQLException ripple across FlightServerTestExtension, OAuthIntegrationTest, and ArrowFlightConnectionConfigImplTest. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eAndPasswordAuth Missed in the previous cleanup commit; this was a ripple from the now- removed throws-SQLException on ArrowFlightConnectionConfigImpl.getPort(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Port 0 is used in several existing tests as a placeholder. It does not cause the async-hang issue this validation was introduced to prevent (that affects truly out-of-range values). Tightening the lower bound to 1 and fixing those tests is left for a follow-up PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Validation done, all tests pass locally. For the validation, I allowed port 0 to be used for compatibility with a few existing tests that used it. I don't think we should allow it though: this is a client code path, it is meaningless to try to connect to port 0. If you agree with this assessment @lidavidm I'll go ahead and create a separate issue+PR for removing 0 from the valid range (we can leave it for a next release). |
|
@ennuite I'm doing a review as well. By the way, we pushed a fix on |
….getPort() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Validate the port synchronously in NettyClientBuilder.build() before calling forAddress(String, int), restoring the fail-fast behaviour that was lost when switching away from forAddress(SocketAddress). Placing the check here protects all callers (not just JDBC), and the resulting IllegalArgumentException is already caught and wrapped as SQLException by ArrowFlightSqlClientHandler.Builder.build(). Reverts the validation that was previously added to ArrowFlightConnectionConfigImpl.getPort() and the associated throws-SQLException ripple across FlightServerTestExtension, OAuthIntegrationTest, and ArrowFlightConnectionConfigImplTest. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eAndPasswordAuth Missed in the previous cleanup commit; this was a ripple from the now- removed throws-SQLException on ArrowFlightConnectionConfigImpl.getPort(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Port 0 is used in several existing tests as a placeholder. It does not cause the async-hang issue this validation was introduced to prevent (that affects truly out-of-range values). Tightening the lower bound to 1 and fixing those tests is left for a follow-up PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dc80adc to
0ee3503
Compare
|
@jbonofre thank you for reviewing! I rebased, let me know of anything else you need and if you have a different opinion on anything that is being discussed. |
What's Changed
The Flight SQL JDBC driver ignored JVM proxy settings (
-Dhttps.proxyHost,-Dhttps.proxyPort). Connections would always go directly to the target host, bypassing any configured proxy.Switched to
NettyChannelBuilder.forAddress(host, port)for the TCP-based schemes. This causes gRPC to go throughProxySelector, which picks up the standard JVM proxy properties.Are these changes tested?
Yes.
Added a test to
ConnectionTestthat installs a recordingProxySelectoras the JVM default, opens a JDBC connection, and asserts thatProxySelector.select()was called. This directly validates that the driver participates in JVM proxy detection without requiring a real proxy server.This change was created with AI assistance (Claude Code). All lines were manually reviewed by a human. The output is not copyrightable subject matter.
Closes #577.