[HIVE-26336] Introduce a new JDBC parameter connectTimeout #3379

pan3793 · 2022-06-16T15:00:28Z

What changes were proposed in this pull request?

Introduce a new JDBC parameter connectTimeout.

Why are the changes needed?

Before HIVE-12371, the Hive JDBC Driver uses DriverManager#loginTimeout as both connectTimeout and socketTimeout, which usually cause socket timeout exception to users who use Hive JDBC Driver in Spring Boot project, because Spring Boot will setLoginTimeout to 30s (default value).

HIVE-12371 introduced a new parameter socketTimeout to replace loginTimeout, and does not care about DriverManager#loginTimeout anymore.

This PR proposes to add a new JDBC parameter connectTimeout

Does this PR introduce any user-facing change?

Yes. New JDBC parameter connectTimeout is introduced.

How was this patch tested?

New test added, and pass the existing UT.

@OverRide

* Source Iceberg PR - Core: Remove deprecated APIs up to 0.13.0 * Revert "HIVE-25563: Iceberg table operations hang a long time if metadata is missing/corrupted (Adam Szita, reviewed by Marton Bod)" - applying instead Hive: Limit number of retries when metadata file is missing (apache#3379) This reverts commit 7b600fe. * Source Iceberg PR - Hive: Limit number of retries when metadata file is missing (apache#3379) * Source Iceberg PR - Hive: Fix RetryingMetaStoreClient for Hive 2.1 (apache#3403) * Source Iceberg PR - Switch from new HashMap to Maps.newHashMap (apache#3648) * Source Iceberg PR - Hive: HiveCatalog should remove HMS stats for certain engines based on config (apache#3652) - Use the Iceberg config property * Source Iceberg PR - Core: If status check fails, commit should be unknown (apache#3717) * Source Iceberg PR - Build: Add checkstyle rule for instantiating HashMap, HashSet, ArrayList (apache#3689) * Source Iceberg PR - Test: Make sure to delete temp folders (apache#3790) * Source Iceberg PR - API: Register existing tables in Iceberg HiveCatalog (apache#3851) * Source Iceberg PR - Hive: Make Iceberg table filter optional in HiveCatalog (apache#3908) * Source Iceberg PR - Core: Add reserved UUID Table Property and Expose in HMS. (apache#3914) * Source Iceberg PR - Hive: Known exception should not become CommitStateUnknownException (apache#4261) * Source Iceberg PR - Build: Add missing @OverRide annotations (apache#3654)

pan3793 · 2022-06-17T11:29:02Z

@prasanthj @pvary would you please take a look?

jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java

pvary · 2022-06-21T09:37:36Z

It would be good to create unit tests to define the expected behaviour and prevent changes in it

pan3793 · 2022-06-21T09:42:32Z

Let me try to add a unit test. (Not sure if easy because it's network-related)

pvary · 2022-06-23T05:45:16Z

itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcTimeout.java

+    Thread conn2 = new Thread(run);
+    conn1.start();
+    conn2.start();
+    assertEquals(goodCounter.get(), 1);


Couldn't this become flaky, if the getConnection is slow?

WIP: I can not compile Hive on M1 machine, just push and finding a x86 machine...

I'm sorry I can not compile and run the test even on an x86 machine, the IDEA is stuck on building hive-exec module... and I simplified the test to check the variable initialization only, please let me know if this is sufficient or not.

I would like to see some test where we make sure that no future change will break the loginTimeout, socketTimeout behaviour.

Not sure about your env, but we sometime create virtual machines and run our tests there to have a linux env. This might help you too if you have access to some env.

pvary · 2022-06-23T05:48:33Z

itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcTimeout.java

+          stmt.execute("SELECT reflect('java.lang.Thread', 'sleep', 3000L)");
+        }
+      } catch (Exception exception) {
+        badCounter.incrementAndGet();


Are we sure that this is the exception what we expect here?

pan3793 · 2022-06-23T09:45:17Z

Passed test on x86 machine, #3377 may helpful for fixing M1 building.

mvn test -Pitests -pl :hive-it-unit,:hive-jdbc -Dtest=org.apache.hive.jdbc.TestJdbcTimeout

[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.hive.jdbc.TestJdbcTimeout
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 23.204 s - in org.apache.hive.jdbc.TestJdbcTimeout
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Hive JDBC 4.0.0-alpha-2-SNAPSHOT:
[INFO]
[INFO] Hive JDBC .......................................... SUCCESS [  4.952 s]
[INFO] Hive Integration - Unit Tests ...................... SUCCESS [ 32.634 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  38.254 s
[INFO] Finished at: 2022-06-23T18:15:33+08:00
[INFO] ------------------------------------------------------------------------

pvary · 2022-06-23T10:25:02Z

itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcTimeout.java

+  }
+
+  @Test
+  public void testLoginTimeoutDoesNotAffectSocketTime() throws Exception {


This test does not fail before the PR

It should fail before HIVE-12371, HIVE-12371 changes behavior but does not provide test

Actually, I don't know how to test loginTimeout, does Hive have something like "session init sql"?

Could you use simple jdbc connection?
miniHS2.getJdbcURL() could provide the jdbc url to connect to the running HS2 server

I guess there is no difference because HiveConnection invokes openSession in the constructor

As for the testing I have found this:
https://stackoverflow.com/questions/100841/artificially-create-a-connection-timeout-error

Not sure if it's suitable, from my understanding, it can be used to test socket timeout but not connect timeout. Because the server must accept the connection and establish a TCP connection before reading a parameter(sleep=6000) passed by HTTP protocol.

I think it make sense to use DriverManager.loginTimeout as connectTimeout but not socketTimeout. Comparing login or connect, a query may take hours, that is why users report socket timeout exceptions.

Was this loginTimeout used anywhere else? Do we need to keep backward compatibility?

As for the testing I have found this:
https://stackoverflow.com/questions/100841/artificially-create-a-connection-timeout-error

Not sure if it's suitable, from my understanding, it can be used to test socket timeout but not connect timeout. Because the server must accept the connection and establish a TCP connection before reading a parameter(sleep=6000) passed by HTTP protocol.

We can have different tests. One for the connectTimeout, and one for the socketTimeout, and set the other to a value which does not interfere with the test.

Was this loginTimeout used anywhere else?

I think no, the IDEA index indicates it is not used by others.

Do we need to keep backward compatibility?

I think not necessary, the old implementation does not right. HIVE-12371 has already broken it one time.

To clarify I think we have 3 approaches for improving the "*timeout" stuff

Introduce new JDBC parameter connectTimeout w/ exsiting socketTimeout, ignore DriverManager.loginTimeout

Same as 1, but use DriverManager.loginTimeout as connectTimeout if connectTimeout absent.

Introduce new JDBC parameter loginTimeout and connectTimeout w/s exsiting socketTime, use loginTimeout as socketTimeout for openSession request, and reset socketTimeout if openSession successful.

Which one do you prefer?

I would go with the 1st approach

Also set the DriverManager.loginTimeout to @deprecated

Thanks for the confirmation, will implement it in the next few days.

itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcTimeout.java

pan3793 · 2022-07-15T07:20:43Z

Introduce new JDBC parameter connectTimeout w/ exsiting socketTimeout, ignore DriverManager.loginTimeout

Implement it as we discussed before, but I found it is not easy to add integration tests.

The following tests are what I want to add at first, but finally, I realized it does not make sense, because JDBC always runs in async mode, a "sleep" query will not block the server return the response immediately.

@Test(expected = SocketTimeoutException.class)
  public void testThrowSocketTimeoutException() throws Exception {
    String url = miniHS2.getJdbcURL("default", "socketTimeout=5000");
    try (HiveConnection conn = (HiveConnection) DriverManager.getConnection(url)) {
      try (Statement stmt = conn.createStatement()) {
        stmt.executeQuery("SELECT reflect('java.lang.Thread', 'sleep', bigint(6000))");
      }
    }
  }

  @Test
  public void testConnectTimeoutDoesNotAffectSocketTime() throws Exception {
    String url = miniHS2.getJdbcURL("default", "connectTimeout=5000");
    try (HiveConnection conn = (HiveConnection) DriverManager.getConnection(url)) {
      try (Statement stmt = conn.createStatement()) {
        stmt.executeQuery("SELECT reflect('java.lang.Thread', 'sleep', bigint(6000))");
      }
    }
  }

### _Why are the changes needed?_ There are two concepts `connect timeout` and `so timeout` in the socket, currently, the Kyuubi Hive JDBC driver uses `DriverManager#loginTimeout` as both of them, it does not make sense, and will cause unexpected "read timeout" exceptions if `DriverManager#loginTimeout` was set a small value, e.g. Spring Boot set it to 30s in default. This PR proposes to introduce two dedicated JDBC parameters to control them. See also apache/hive#3379 ### _How was this patch tested?_ - [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [x] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes #3152 from pan3793/timeout. Closes #3152 5dcae66 [Cheng Pan] nit 816a851 [Cheng Pan] nit 3b5e6ce [Cheng Pan] fix npe 4cb73df [Cheng Pan] Introduce JDBC parameters to control connection timeout Authored-by: Cheng Pan <chengpan@apache.org> Signed-off-by: Cheng Pan <chengpan@apache.org>

github-actions · 2022-09-25T00:24:46Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.

@OverRide

* Source Iceberg PR - Core: Remove deprecated APIs up to 0.13.0 * Revert "HIVE-25563: Iceberg table operations hang a long time if metadata is missing/corrupted (Adam Szita, reviewed by Marton Bod)" - applying instead Hive: Limit number of retries when metadata file is missing (apache#3379) This reverts commit 7b600fe. * Source Iceberg PR - Hive: Limit number of retries when metadata file is missing (apache#3379) * Source Iceberg PR - Hive: Fix RetryingMetaStoreClient for Hive 2.1 (apache#3403) * Source Iceberg PR - Switch from new HashMap to Maps.newHashMap (apache#3648) * Source Iceberg PR - Hive: HiveCatalog should remove HMS stats for certain engines based on config (apache#3652) - Use the Iceberg config property * Source Iceberg PR - Core: If status check fails, commit should be unknown (apache#3717) * Source Iceberg PR - Build: Add checkstyle rule for instantiating HashMap, HashSet, ArrayList (apache#3689) * Source Iceberg PR - Test: Make sure to delete temp folders (apache#3790) * Source Iceberg PR - API: Register existing tables in Iceberg HiveCatalog (apache#3851) * Source Iceberg PR - Hive: Make Iceberg table filter optional in HiveCatalog (apache#3908) * Source Iceberg PR - Core: Add reserved UUID Table Property and Expose in HMS. (apache#3914) * Source Iceberg PR - Hive: Known exception should not become CommitStateUnknownException (apache#4261) * Source Iceberg PR - Build: Add missing @OverRide annotations (apache#3654)

@OverRide

* Source Iceberg PR - Core: Remove deprecated APIs up to 0.13.0 * Revert "HIVE-25563: Iceberg table operations hang a long time if metadata is missing/corrupted (Adam Szita, reviewed by Marton Bod)" - applying instead Hive: Limit number of retries when metadata file is missing (apache#3379) This reverts commit 7b600fe. * Source Iceberg PR - Hive: Limit number of retries when metadata file is missing (apache#3379) * Source Iceberg PR - Hive: Fix RetryingMetaStoreClient for Hive 2.1 (apache#3403) * Source Iceberg PR - Switch from new HashMap to Maps.newHashMap (apache#3648) * Source Iceberg PR - Hive: HiveCatalog should remove HMS stats for certain engines based on config (apache#3652) - Use the Iceberg config property * Source Iceberg PR - Core: If status check fails, commit should be unknown (apache#3717) * Source Iceberg PR - Build: Add checkstyle rule for instantiating HashMap, HashSet, ArrayList (apache#3689) * Source Iceberg PR - Test: Make sure to delete temp folders (apache#3790) * Source Iceberg PR - API: Register existing tables in Iceberg HiveCatalog (apache#3851) * Source Iceberg PR - Hive: Make Iceberg table filter optional in HiveCatalog (apache#3908) * Source Iceberg PR - Core: Add reserved UUID Table Property and Expose in HMS. (apache#3914) * Source Iceberg PR - Hive: Known exception should not become CommitStateUnknownException (apache#4261) * Source Iceberg PR - Build: Add missing @OverRide annotations (apache#3654)

shalk · 2023-04-07T09:07:39Z

i think this pr can be reopen. in somecase connection timeout is different from socketTimeout.

kgyrtkirk added tests pending tests failed tests passed and removed tests pending tests failed labels Jun 16, 2022

pvary reviewed Jun 21, 2022

View reviewed changes

jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java Outdated Show resolved Hide resolved

kgyrtkirk added tests pending and removed tests passed labels Jun 23, 2022

pvary reviewed Jun 23, 2022

View reviewed changes

kgyrtkirk added tests unstable and removed tests pending labels Jun 23, 2022

pvary reviewed Jun 23, 2022

View reviewed changes

itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcTimeout.java Outdated Show resolved Hide resolved

kgyrtkirk added tests pending tests failed and removed tests unstable tests pending tests failed labels Jun 23, 2022

[HIVE-26336] Introduce connectTimeout as JDBC url parameter

cdf1872

pan3793 force-pushed the timeout branch from 2f184ce to cdf1872 Compare July 15, 2022 07:15

kgyrtkirk added tests pending and removed tests failed labels Jul 15, 2022

kgyrtkirk added tests passed and removed tests pending labels Jul 15, 2022

pan3793 changed the title ~~[HIVE-26336] Hive JDBC Driver should respect JDBC DriverManager#loginTimeout~~ [HIVE-26336] Introduce a new JDBC parameter loginTimeout Jul 21, 2022

pan3793 changed the title ~~[HIVE-26336] Introduce a new JDBC parameter loginTimeout~~ [HIVE-26336] Introduce a new JDBC parameter connectTimeout Jul 26, 2022

pan3793 mentioned this pull request Jul 26, 2022

Introduce JDBC parameters to control connection timeout apache/kyuubi#3152

Closed

3 tasks

github-actions bot added the stale label Sep 25, 2022

github-actions bot closed this Oct 2, 2022

pan3793 mentioned this pull request Mar 24, 2023

HIVE-27172: Add the HMS client connection timeout config #4150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HIVE-26336] Introduce a new JDBC parameter connectTimeout #3379

[HIVE-26336] Introduce a new JDBC parameter connectTimeout #3379

pan3793 commented Jun 16, 2022 •

edited

pan3793 commented Jun 17, 2022

pvary commented Jun 21, 2022

pan3793 commented Jun 21, 2022

pvary Jun 23, 2022

pan3793 Jun 23, 2022

pan3793 Jun 23, 2022

pvary Jun 23, 2022

pvary Jun 23, 2022

pan3793 commented Jun 23, 2022 •

edited

pvary Jun 23, 2022

pan3793 Jun 23, 2022

pan3793 Jun 23, 2022

pvary Jun 23, 2022

pan3793 Jun 23, 2022

pan3793 Jun 24, 2022

pvary Jun 24, 2022

pan3793 Jun 24, 2022 •

edited

pvary Jun 27, 2022 •

edited

pan3793 Jun 27, 2022

pan3793 commented Jul 15, 2022 •

edited

github-actions bot commented Sep 25, 2022

shalk commented Apr 7, 2023

[HIVE-26336] Introduce a new JDBC parameter connectTimeout #3379

[HIVE-26336] Introduce a new JDBC parameter connectTimeout #3379

Conversation

pan3793 commented Jun 16, 2022 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

pan3793 commented Jun 17, 2022

pvary commented Jun 21, 2022

pan3793 commented Jun 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan3793 commented Jun 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan3793 Jun 24, 2022 • edited

Choose a reason for hiding this comment

pvary Jun 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan3793 commented Jul 15, 2022 • edited

github-actions bot commented Sep 25, 2022

shalk commented Apr 7, 2023

pan3793 commented Jun 16, 2022 •

edited

pan3793 commented Jun 23, 2022 •

edited

pan3793 Jun 24, 2022 •

edited

pvary Jun 27, 2022 •

edited

pan3793 commented Jul 15, 2022 •

edited