HIVE-12371 Adding a timeout connection parameter for JDBC #1611
Conversation
|
The failed test seemes to be unralated |
jshmchenxi
force-pushed
the
HIVE-12371
branch
from
e62ce78 to
5c23fec
Compare
kgyrtkirk
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
jshmchenxi
force-pushed
the
HIVE-12371
branch
from
5c23fec to
c66dce1
Compare
kgyrtkirk
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
| private void setupLoginTimeout() { | ||
| long timeOut = TimeUnit.SECONDS.toMillis(DriverManager.getLoginTimeout()); | ||
| String socketTimeoutStr = sessConfMap.getOrDefault(JdbcConnectionParams.SOCKET_TIMEOUT, "0"); | ||
| long timeOut = 0; |
There was a problem hiding this comment.
What if the default value set to TimeUnit.SECONDS.toMillis(DriverManager.getLoginTimeout())?
There was a problem hiding this comment.
This issue describes the problem: HIVE-22196 : Socket timeouts happen when other drivers set DriverManager.loginTimeout.
There was a problem hiding this comment.
Set the default value to 0 may cause problems for those elder clients using DriverManager.loginTimeout to prevent the client from hang or return quickly if there are some problems in HS2, users have to refactor the uri with the socket timeout parameter after applying this. After all, there are some preconditions should be met on the HIVE-22196: 1, There are different drivers that created in the same jvm; 2, One of the driver sets the loginTimeout that is inappropriate for others.
There was a problem hiding this comment.
Sorry for the late reply. If users depends on DriverManager.loginTimeout to control HS2 jdbc timeout, they will have to refactor their code after applying this. However, we haven't yet met a situation that we need this timeout to return quickly when HS2 hangs or something. Our problem is when submitting a big query which needs minutes to compile, the connection is lost because of this DriverManager.loginTimeout. Maybe we need to set a timeout parameter for HS2 on the server side too.
|
+1, looks good to me! |
jshmchenxi
force-pushed
the
HIVE-12371
branch
from
c66dce1 to
d148732
Compare
|
@dengzhhu653 Thanks for the review. |
|
Hey @jshmchenxi, you can reopen the pr or push a commit to trigger a new clean build... |
jshmchenxi
force-pushed
the
HIVE-12371
branch
from
d148732 to
2d5ebda
Compare
|
@dengzhhu653 Yes, I rebased the code. The errors seems to be unrelated |
|
lgtm, +1 |
Co-authored-by: Xi Chen <chenxi07@qiyi.com>
Co-authored-by: Xi Chen <chenxi07@qiyi.com>
| private void setupLoginTimeout() { | ||
| long timeOut = TimeUnit.SECONDS.toMillis(DriverManager.getLoginTimeout()); | ||
| String socketTimeoutStr = sessConfMap.getOrDefault(JdbcConnectionParams.SOCKET_TIMEOUT, "0"); |
There was a problem hiding this comment.
String socketTimeoutStr = sessConfMap.getOrDefault(JdbcConnectionParams.SOCKET_TIMEOUT, String.valueOf(DriverManager.getLoginTimeout()));
|
Hi, too late to the party, but i would have continue using PD: Well 4.0 is still on beta so maybe not too late? |
This way we would honour the previous behaviour if the new specific jdbc option is not set. Users may want to configure the timeout globally as before apache#1611
What changes were proposed in this pull request?
Improvement HIVE-12371: Adding a timeout connection parameter for JDBC
Why are the changes needed?
We run into the problem of HIVE-22196: Socket timeouts happen when other drivers set DriverManager.loginTimeout.
It is a good idea to add timeout parameter to hiveserver2 jdbc url. So that we do not use the global DriverManager.loginTimeout which may be unexpected set by other components in the program.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Tested by local install jar