HIVE-27172: Add the HMS client connection timeout config #4150
Conversation
| public static void setUp() throws Exception { | ||
| @Before | ||
| public void setUp() throws Exception { | ||
| HMSHandler.testTimeoutEnabled = true; |
There was a problem hiding this comment.
Nit: This test code hacks HMSHandler class, I will refactor this in a new PR.
There was a problem hiding this comment.
do we need to repeat the whole setup for every test or it's possible to extract just the HMSHandler part?
There was a problem hiding this comment.
Addressed! Have separated the start of MetaStore Server into a BeforeClass method.
|
A similar PR #3379 for JDBC thrift client, IMO the connection timeout is quite important, please help reopen the closed PR if any committer think it's useful. |
| "has an infinite lifetime."), | ||
| CLIENT_SOCKET_TIMEOUT("metastore.client.socket.timeout", "hive.metastore.client.socket.timeout", 600, | ||
| TimeUnit.SECONDS, "MetaStore Client socket timeout in seconds"), | ||
| CLIENT_CONNECTION_TIMEOUT("metastore.client.connection.timeout", "hive.metastore.client.connection.timeout", 10, |
There was a problem hiding this comment.
It makes sense to set connection timeout a small value, because generally, establish the connection is quick, and small timeout make it quick failover if one HMS is down, but it silent changes the default behavior, I'm not sure if such change is acceptable in Hive community
There was a problem hiding this comment.
Nice catch, if there is any concern I will keep the default connection timeout value same as socket timeout.
There was a problem hiding this comment.
@wecharyu, won't be socketTimeout = connectTimeout if not explicitly set?
public TSocket(TConfiguration config, String host, int port, int timeout) {
this(config, host, port, timeout, timeout);
}
public TSocket(TConfiguration config, String host, int port, int socketTimeout, int connectTimeout) {}
There was a problem hiding this comment.
Yes, I have changed the default connectionTimeout to be the same as socketTimeout. But IMHO it's not a good practice because if we set socketTimeout and not set connectionTimeout, the default long time will be gained for connection, I think we should change the behavior by default where socketTimeout = connectionTimeout.
|
Kudos, SonarCloud Quality Gate passed!
|
|
@ayushtkn @deniskuzZ @kasakrisz: Could you help review this PR? |
|
@zabetak @dengzhhu653: Could you also take a look? |
kgyrtkirk
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
| try { | ||
| client.createDatabase(db); | ||
| } catch (MetaException e) { | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Because we have changed to use the remote MetaStore server in this test class, so the timeout exception was wrapped in TTransportException rather than MetaException.
|
Kudos, SonarCloud Quality Gate passed!
|
…reviewed by Cheng Pan, Denys Kuzmenko) Closes apache#4150
…reviewed by Cheng Pan, Denys Kuzmenko) Closes apache#4150
4 participants
What changes were proposed in this pull request?
Currently
HiveMetaStoreClientuseCLIENT_SOCKET_TIMEOUTas both socket timeout and connection timeout, we want to introduce a new HiveMetaStoreClient config for connection timeout:Why are the changes needed?
Does this PR introduce any user-facing change?
Yes, user can set the client connection timeout by this conf.
How was this patch tested?
Add a unit test.