Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HIVE-12371 Adding a timeout connection parameter for JDBC #1611

Merged
merged 1 commit into from Feb 16, 2021

Conversation

jshmchenxi
Copy link
Contributor

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

@jshmchenxi
Copy link
Contributor Author

The failed test seemes to be unralated

private void setupLoginTimeout() {
long timeOut = TimeUnit.SECONDS.toMillis(DriverManager.getLoginTimeout());
String socketTimeoutStr = sessConfMap.getOrDefault(JdbcConnectionParams.SOCKET_TIMEOUT, "0");
long timeOut = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the default value set to TimeUnit.SECONDS.toMillis(DriverManager.getLoginTimeout())?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue describes the problem: HIVE-22196 : Socket timeouts happen when other drivers set DriverManager.loginTimeout.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dengzhhu653
Copy link
Member

+1, looks good to me!

@jshmchenxi
Copy link
Contributor Author

@dengzhhu653 Thanks for the review.
If this patch is merged, hive document needs to update for the new JDBC parameter.

@dengzhhu653
Copy link
Member

Hey @jshmchenxi, you can reopen the pr or push a commit to trigger a new clean build...

@jshmchenxi
Copy link
Contributor Author

@dengzhhu653 Yes, I rebased the code. The errors seems to be unrelated

@prasanthj
Copy link
Contributor

lgtm, +1

@prasanthj prasanthj merged commit 7935534 into apache:master Feb 16, 2021
aihuaxu pushed a commit to aihuaxu/hive that referenced this pull request Mar 17, 2021
amanraj2520 pushed a commit to amanraj2520/hive that referenced this pull request Aug 14, 2023
dengzhhu653 pushed a commit that referenced this pull request Aug 17, 2023
private void setupLoginTimeout() {
long timeOut = TimeUnit.SECONDS.toMillis(DriverManager.getLoginTimeout());
String socketTimeoutStr = sessConfMap.getOrDefault(JdbcConnectionParams.SOCKET_TIMEOUT, "0");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String socketTimeoutStr = sessConfMap.getOrDefault(JdbcConnectionParams.SOCKET_TIMEOUT, String.valueOf(DriverManager.getLoginTimeout()));

@jneira-stratio
Copy link

jneira-stratio commented Dec 15, 2023

Hi, too late to the party, but i would have continue using DriverManager.getLoginTimeout() as fallback if the property socketTimeout is not set to keep backwards compatibility with users using the generic option to alter hive jdbc on purpose.
//cc @prasanthj

PD: Well 4.0 is still on beta so maybe not too late?

jneira-stratio added a commit to jneira-stratio/hive that referenced this pull request Dec 20, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants