You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/01/20 06:09:57 UTC
[GitHub] [iceberg] Jecarm opened a new pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Jecarm opened a new pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119
#### This PR is to fix issue [#1994](https://github.com/apache/iceberg/issues/1994).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r565059131
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -51,4 +81,918 @@ protected void close(Object client) {
}
}
+
+ private static class MockHiveClientPool extends ClientPool<IMetaStoreClient, Exception> {
Review comment:
Shall we just extend the HiveClientPool here? Or we want to test the ClientPool too?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#issuecomment-766750008
Thanks for the changes @Jecarm! The actual code part looks good to me.
Could we add some tests for this code path? Maybe using some MockClients to check it the reconnect is attempted when needed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#issuecomment-773940415
@Jecarm: I really appreciate your work and sorry for being slow with the review (had to stop because of a meeting). I feel that we are almost there.
Thanks,
Peter
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary merged pull request #2119: Hive: Fix connection pool fails to reconnect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary merged pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570822338
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
Review comment:
the exception of `MetaException` or `TTransportException` for `getAllFunctions` in this method will not be triggered. If need test `TTransportException`, we will create a new client In a separate method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r562320456
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
The message of `TTransportException` wrapped by `MetaException` is as follows:
```
ERROR - Got exception: org.apache.thrift.transport.TTransportException null
org.apache.thrift.transport.TTransportException: null at
org.apache.thrift.transport.TIOStreamTransport.read(TIOStreamTransport.java:132) ~[libthrift-0.9.3.jar:0.9.3] at
org.apache.thrift.transport.TTransport.readAll(TTransport.java:86) ~[libthrift-0.9.3.jar:0.9.3] at
org.apache.thrift.protocol.TBinaryProtocol.readAll(TBinaryProtocol.java:429) ~[libthrift-0.9.3.jar:0.9.3] at
org.apache.thrift.protocol.TBinaryProtocol.readI32(TBinaryProtocol.java:318) ~[libthrift-0.9.3.jar:0.9.3] at
org.apache.thrift.protocol.TBinaryProtocol.readMessageBegin(TBinaryProtocol.java:219) ~[libthrift-0.9.3.jar:0.9.3] at
org.apache.thrift.TServiceClient.receiveBase(TServiceClient.java:77) ~[libthrift-0.9.3.jar:0.9.3] at
org.apache.hadoop.hive.metastore.api.ThriftHiveMetastore$Client.recv_get_all_tables(ThriftHiveMetastore.java:1487) ~[hive-metastore-2.3.7.jar:2.3.7] at
org.apache.hadoop.hive.metastore.api.ThriftHiveMetastore$Client.get_all_tables(ThriftHiveMetastore.java:1474) ~[hive-metastore-2.3.7.jar:2.3.7] at
org.apache.hadoop.hive.metastore.HiveMetaStoreClient.getAllTables(HiveMetaStoreClient.java:1451) [hive-metastore-2.3.7.jar:2.3.7]
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r571942321
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -51,4 +81,918 @@ protected void close(Object client) {
}
}
+
+ private static class MockHiveClientPool extends ClientPool<IMetaStoreClient, Exception> {
+
+ MockHiveClientPool(int poolSize, Class<? extends Exception> reconnectExc) {
+ super(poolSize, reconnectExc);
+ }
+
+ @Override
+ protected IMetaStoreClient newClient() {
+ return new MockMetaStoreFailureClient();
Review comment:
Looks good. Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r563569336
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/ClientPool.java
##########
@@ -54,7 +54,7 @@
return action.run(client);
} catch (Exception exc) {
- if (reconnectExc.isInstance(exc)) {
+ if (reconnectExc.isInstance(exc) || failureDetection(exc)) {
Review comment:
Yes, it can be done, but the constructor of `ClientPool` may change. Because the members of `reconnectExc` will be deleted. Does this affect other component calls？
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r563599761
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/ClientPool.java
##########
@@ -54,7 +54,7 @@
return action.run(client);
} catch (Exception exc) {
- if (reconnectExc.isInstance(exc)) {
+ if (reconnectExc.isInstance(exc) || failureDetection(exc)) {
Review comment:
Or just move reconnectExc.isInstance(exc) into the failureDetection(exc) method in `ClientPool`, like this:
```
try {
return action.run(client);
} catch (Exception exc) {
if (failureDetection(exc)) {
...
}
}
protected boolean failureDetection(Exception exc) {
return reconnectExc.isInstance(exc);
}
```
@pvary
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r560715337
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
+ if (null != ex && ex instanceof MetaException) {
+ try {
+ client.getDatabase("default");
Review comment:
I would prefer to have a configurable timeout for this, so we do not end up doubling every failed metastore call. MetaException can be returned even if the HMS is working correctly. Also it would be good to be able to turn off this check with the same config.
One more thing to consider is the time of the duplicated call. Would it be possible to return the original result and do the check asynchronously? Or in case of failure we always retry now?
If I remember correctly theoretically you can have a HMS without a "default" db.
I remember that there was an API call to get the HMS version which should be good, or if it does not exist in Hive 2.3 then maybe `get_current_notificationEventId` would be good for the quick health check.
What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r561483981
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
Personally, I support this solution which is to detect the MetaException that wraps TTransportException and return true.
If we can reach an agreement, I will do it in this way.
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
The message of `TTransportException` wrapped by `MetaException` is as follows:
```
ERROR - Got exception: org.apache.thrift.transport.TTransportException null
org.apache.thrift.transport.TTransportException: null at
org.apache.thrift.transport.TIOStreamTransport.read(TIOStreamTransport.java:132) ~[libthrift-0.9.3.jar:0.9.3] at
org.apache.thrift.transport.TTransport.readAll(TTransport.java:86) ~[libthrift-0.9.3.jar:0.9.3] at
org.apache.thrift.protocol.TBinaryProtocol.readAll(TBinaryProtocol.java:429) ~[libthrift-0.9.3.jar:0.9.3] at
org.apache.thrift.protocol.TBinaryProtocol.readI32(TBinaryProtocol.java:318) ~[libthrift-0.9.3.jar:0.9.3] at
org.apache.thrift.protocol.TBinaryProtocol.readMessageBegin(TBinaryProtocol.java:219) ~[libthrift-0.9.3.jar:0.9.3] at
org.apache.thrift.TServiceClient.receiveBase(TServiceClient.java:77) ~[libthrift-0.9.3.jar:0.9.3] at
org.apache.hadoop.hive.metastore.api.ThriftHiveMetastore$Client.recv_get_all_tables(ThriftHiveMetastore.java:1487) ~[hive-metastore-2.3.7.jar:2.3.7] at
org.apache.hadoop.hive.metastore.api.ThriftHiveMetastore$Client.get_all_tables(ThriftHiveMetastore.java:1474) ~[hive-metastore-2.3.7.jar:2.3.7] at
org.apache.hadoop.hive.metastore.HiveMetaStoreClient.getAllTables(HiveMetaStoreClient.java:1451) [hive-metastore-2.3.7.jar:2.3.7]
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] marton-bod commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
marton-bod commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570975264
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,36 +19,88 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Function;
+import org.apache.hadoop.hive.metastore.api.FunctionType;
+import org.apache.hadoop.hive.metastore.api.GetAllFunctionsResponse;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.PrincipalType;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.thrift.transport.TTransportException;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
- try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
- pool.run(Object::toString);
- }
+ Mockito.doThrow(new RuntimeException()).when(clients).newClient();
+ clients.run(Object :: toString);
}
- private static class MockClientPool extends ClientPool<Object, Exception> {
+ @Test(expected = MetaException.class)
Review comment:
+1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r564236865
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,15 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(Exception e) {
+ if (super.failureDetection(e) || (e != null && e instanceof MetaException &&
+ e.getMessage().contains("Got exception: org.apache.thrift.transport.TTransportException"))) {
Review comment:
It's solved.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r563607914
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/ClientPool.java
##########
@@ -54,7 +54,7 @@
return action.run(client);
} catch (Exception exc) {
- if (reconnectExc.isInstance(exc)) {
+ if (reconnectExc.isInstance(exc) || failureDetection(exc)) {
Review comment:
@Jecarm: This is what I have been thinking about too.
And in `HiveClientPool` we can do something like this:
```
@Override
protected boolean failureDetection(Exception e) {
if (super.failureDetection(e) || (e != null && e instanceof MetaException &&
e.getMessage().contains("Got exception: org.apache.thrift.transport.TTransportException"))) {
return true;
}
return false;
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r561483981
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
Personally, I support this solution which is to detect the MetaException that wraps TTransportException and return true.
If we can reach an agreement, I will do it in this way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r561585391
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
@pvary, what do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on pull request #2119: Hive: Fix connection pool fails to reconnect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#issuecomment-775755929
Thanks for a job well done @Jecarm, and @rdblue, @marton-bod for the review!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#issuecomment-767712301
> > Could we add some tests for this code path? Maybe using some MockClients to check it the reconnect is attempted when needed?
>
> I agree that it would be great to test this code path, but adding the mock probably isn't that straightforward. Do we have existing mock tests for this that could be adapted? If not, we may want to get it in and then follow up with tests later.
Extending the HiveCatalog and overriding the `newClient` method to return a mockClient which throws specific exceptions on specific methods seems like a reasonable effort for preserving the functionality against unintended changes. Would this become more complicated or more brittle than it seems for the first glance @Jecarm?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570831280
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
+ }
+
+ @Test(expected = MetaException.class)
+ public void testConnectionFailure() throws Exception {
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+ Mockito.doThrow(new MetaException("Another meta exception"))
+ .when(hmsClient).getTables(Mockito.anyString(), Mockito.anyString());
+ clients.run(client -> client.getTables("default", "t"));
+ }
+
private static class MockClientPool extends ClientPool<Object, Exception> {
Review comment:
I will clear it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r560715337
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
+ if (null != ex && ex instanceof MetaException) {
+ try {
+ client.getDatabase("default");
Review comment:
I would prefer to have a configurable timeout for this, so we do not end up doubling every failed metastore call. MetaException can be returned even if the HMS is working correctly. Also it would be good to be able to turn off this check with the same config.
One more thing to consider is the time of the duplicated call. Would it be possible to return the original result and do the check asynchronously? Or in case of failure we always retry now?
If I remember correctly theoretically you can habe a HMS without a "default" db.
I remember that there was an API call to get the HMS version which should be good, or if it does not exist in Hive 2.3 then maybe `get_current_notificationEventId` would be good for the quick health check.
What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r563562854
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +84,15 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(Exception e) {
+ if (Objects.nonNull(e) && e instanceof MetaException &&
Review comment:
I would replace it with `e != null`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r567788926
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -51,4 +81,918 @@ protected void close(Object client) {
}
}
+
+ private static class MockHiveClientPool extends ClientPool<IMetaStoreClient, Exception> {
+
+ MockHiveClientPool(int poolSize, Class<? extends Exception> reconnectExc) {
+ super(poolSize, reconnectExc);
+ }
+
+ @Override
+ protected IMetaStoreClient newClient() {
+ return new MockMetaStoreFailureClient();
Review comment:
@Jecarm: I would do something like this:
```
@Override
protected HiveMetaStoreClient newClient() {
HiveMetaStoreClient client = Mockito.mock(HiveMetaStoreClient.class);
try {
Mockito.doThrow(TTransportException.class).when(client).getAllDatabases();
Mockito.doThrow(new MetaException("Got exception: org.apache.thrift.transport.TTransportException")).when(client).getAllFunctions();
Mockito.doThrow(new MetaException("Another meta exception")).when(client).getTables(Mockito.anyString(), Mockito.anyString());
} catch (Exception e) {
throw new RuntimeException("This should not happen", e);
}
return client;
}
@Override
protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
try {
Mockito.doReturn(new ArrayList<>()).when(newClient).getAllDatabases();
Mockito.doReturn(new GetAllFunctionsResponse()).when(newClient).getAllFunctions();
Mockito.doReturn(new ArrayList<>()).when(newClient).getAllTables(Mockito.anyString());
} catch (Exception e) {
throw new RuntimeException("RE", e);
}
return newClient;
}
```
After more thought I think checking the number of reconnection calls through the number of calls on the mock objects could be misleading, so I think we could check them in the `MockClientPool` in a counter / boolean if we do not find a more elegant way.
Updated: IMetasStoreClient -> HiveMetaStoreClient
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r567802401
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -51,4 +81,918 @@ protected void close(Object client) {
}
}
+
+ private static class MockHiveClientPool extends ClientPool<IMetaStoreClient, Exception> {
+
+ MockHiveClientPool(int poolSize, Class<? extends Exception> reconnectExc) {
+ super(poolSize, reconnectExc);
+ }
+
+ @Override
+ protected IMetaStoreClient newClient() {
+ return new MockMetaStoreFailureClient();
Review comment:
Maybe something like this:
```
private static class MockHiveClientPool extends HiveClientPool {
public int reconnectCalls = 0;
MockHiveClientPool() {
super(2, new HiveConf());
}
@Override
protected HiveMetaStoreClient newClient() {
[...]
}
@Override
protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
HiveMetaStoreClient client2 = Mockito.mock(HiveMetaStoreClient.class);
[..]
}
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r563620153
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/ClientPool.java
##########
@@ -54,7 +54,7 @@
return action.run(client);
} catch (Exception exc) {
- if (reconnectExc.isInstance(exc)) {
+ if (reconnectExc.isInstance(exc) || failureDetection(exc)) {
Review comment:
Okay.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r571942823
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
Review comment:
Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] marton-bod commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
marton-bod commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r571916155
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,15 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean isConnectionException(Exception e) {
+ if (super.isConnectionException(e) || (e != null && e instanceof MetaException &&
+ e.getMessage().contains("Got exception: org.apache.thrift.transport.TTransportException"))) {
Review comment:
What do you mean, how does it impact debugging? E.g. not being able to put a breakpoint there? I don't think we should optimize for that in the source code - less verbose is usually better. Plus for debugging purposes, you can always stop at the return line and evaluate the boolean expression in your IDE, or just see what which branch the execution follows after `isConnectionException()`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570806151
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
Review comment:
i will fix them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r564158952
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,15 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(Exception e) {
Review comment:
How about renaming this to `isConnectionException`? I think that is more clear about what the returned value is than `failureDetection`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r572631378
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,15 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean isConnectionException(Exception e) {
+ if (super.isConnectionException(e) || (e != null && e instanceof MetaException &&
+ e.getMessage().contains("Got exception: org.apache.thrift.transport.TTransportException"))) {
Review comment:
i fixed it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r564158456
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,15 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(Exception e) {
+ if (super.failureDetection(e) || (e != null && e instanceof MetaException &&
+ e.getMessage().contains("Got exception: org.apache.thrift.transport.TTransportException"))) {
Review comment:
Nit: continuation indents are 2 indents / 4 spaces.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570830454
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
+ }
+
+ @Test(expected = MetaException.class)
+ public void testConnectionFailure() throws Exception {
Review comment:
like this:
@Test(expected = MetaException.class)
public void testNewClientFailure() throws Exception {
HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
Mockito.doReturn(hmsClient).when(clients).newClient();
Mockito.doThrow(new MetaException("Another meta exception"))
.when(hmsClient).getTables(Mockito.anyString(), Mockito.anyString());
clients.run(client -> client.getTables("default", "t"));
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570828993
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
+ }
+
+ @Test(expected = MetaException.class)
+ public void testConnectionFailure() throws Exception {
Review comment:
Can we use `testConnectionFailure` test method implementation instead of `testNewClientFailure` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r562819275
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
That seems pretty specific to me: `Got exception: org.apache.thrift.transport.TTransportException`. @pvary, would you be okay with matching that string?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#issuecomment-766750008
Thanks for the changes @Jecarm! The actual code part looks good to me.
Could we add some tests for this code path? Maybe using some MockClients to check it the reconnect is attempted when needed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570823834
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
Review comment:
not necessary
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570819107
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
Review comment:
Should we check?
```
Mockito.verify(clients).reconnect(hmsClient);
Mockito.verify(clients, never()).reconnect(hmsClient);
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r571944539
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,15 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean isConnectionException(Exception e) {
+ if (super.isConnectionException(e) || (e != null && e instanceof MetaException &&
+ e.getMessage().contains("Got exception: org.apache.thrift.transport.TTransportException"))) {
Review comment:
Thanks @Jecarm for all the fixes.
For me it seems that @marton-bod has a good point. I think this is the last thing and then I would happily push the merge! 😄 Thanks for all your work on this!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r563432492
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
I have submitted the new code. Please review it. @pvary @rdblue
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570796695
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
Review comment:
nit: Please import concrete classes
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570817875
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
Review comment:
I think it is perfectly ok to test only with a single method with one type of exception.
Do we want to test failures with `new TTransportException` as well?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r560731167
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
+ if (null != ex && ex instanceof MetaException) {
+ try {
+ client.getDatabase("default");
Review comment:
1. "I would prefer to have a configurable timeout for this", This is worth it.
2. "MetaException can be returned even if the HMS is working correctly", For this, the implement does not affect the normal exception `MetaException ` thrown, because when the method returns false, the exception is thrown by `ClientPool.run`.
3. "it would be good to be able to turn off this check with the same config", for this, i don't think it's necessary, because query exception is not acceptable for users. So we need to achieve automatic recovery, is not it?
4. “If I remember correctly theoretically you can have a HMS without a`default`db.”， for this problem, the implement only return false when the exception is not the `TTransportException`, whether the operation `client.getDatabase("default")` is OK or not.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r560809477
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
+ if (null != ex && ex instanceof MetaException) {
+ try {
+ client.getDatabase("default");
Review comment:
2. For example: When you try to run `client.getTable(null, "tableName")` you will receive MetaException. You can take a look at the test cases [here](https://github.com/apache/hive/tree/master/standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/client) and see that there are multiple cases where MetaException is thrown. In this case MetaException is an acceptable result returning a valid error, and we do not need to try to reconnect
3. Based on 2. we might get a MetaException even if everything is working correctly
4. Collecting db information is more costly than returning the only row from a table. We need a check method which is as cheap as possible.
So basically I think we can not rule out receiving the MetaException when we are working correctly. We might decide that in relation to Iceberg we think the risk is minimal enough to accept the risk of double calls and the risk worth the code simplicity. How complicated would be to create a "theoretically" sound solution?
Checked the `client.getServerVersion()` client method. This might be less costly than calling `client.getDatabase("default")`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r561690101
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
MetaException does not have an embedded Exception (no `ex.getCause()` because the MetaException is declared on the Thrift interface and only has a message field) 😢. Also it is used for signaling connection problems and input data issues as well, and the only difference between those is the exception message text. So we can only check the message text to decide on the cause which is too brittle for my taste.
So I would prefer a solution where we do not try to differentiate between the MetaExceptions parsing the text message.
Since we can not have a definite answer when the error is caused by a connection problem or a bad parametrization of the HMS call, I think reconnection every time when we have a MetaException should be avoided if possible. I would use a cheap probe to check the liveliness of the connection (for example `client.getServerVersion()`)
I see the following possibilities:
- Periodic check for the connection - Fix number of extra HMS calls, no extra delay on other HMS calls
- Check when there is a MetaException - Double HMS calls for every call where the response is MetaException
- Some combination of the above
The decision should depend on the use-cases we would like to cover.
- Who will use the `ClientPool`?
- How likely that we issue calls which are badly parametrized?
- How likely that we will have connection issues?
To answer the questions from the Hive side:
- I do not see places where we can have MetaException because of the wrong parametrization of the calls (if so we have to fix it 😄)
- If there is a MetaException then that is because of the connection issues
OTOH I consider ClientPool as an API which could/should be used by other components, and such an API should handle errors better. I would accept the extra complexity to avoid extra `reconnect` attempts, and even maybe extra HMS calls too, but can be convinced otherwise.
That's my 2 cents.
Peter
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r567217920
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -51,4 +81,918 @@ protected void close(Object client) {
}
}
+
+ private static class MockHiveClientPool extends ClientPool<IMetaStoreClient, Exception> {
+
+ MockHiveClientPool(int poolSize, Class<? extends Exception> reconnectExc) {
+ super(poolSize, reconnectExc);
+ }
+
+ @Override
+ protected IMetaStoreClient newClient() {
+ return new MockMetaStoreFailureClient();
Review comment:
I tried it with `Mockito`, like this:
```
HiveClientPool clients = Mockito.mock(HiveClientPool.class);
Mockito.doReturn(Lists.newArrayList("t1", "t2")).when(clients)
.run(client -> client.getAllTables(Mockito.any(String.class)));
hiveCatalog.listTables(Namespace.of("db1"))
```
However, the mock result is `null` when call `.run(client -> client.getAllTables(Mockito.any(String.class)))`.
When i use `.run(Mockito.any(ClientPool.Action.class))` instead of `.run(client -> client.getAllTables(Mockito.any(String.class)))` , the reult is OK.
So, How should i test the different client APIs @pvary ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r563432492
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
I have submitted the new code. Please review it. @pvary @rdblue
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +84,15 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(Exception e) {
+ if (Objects.nonNull(e) && e instanceof MetaException &&
Review comment:
I would replace it with `e != null`
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/ClientPool.java
##########
@@ -54,7 +54,7 @@
return action.run(client);
} catch (Exception exc) {
- if (reconnectExc.isInstance(exc)) {
+ if (reconnectExc.isInstance(exc) || failureDetection(exc)) {
Review comment:
Yes, it can be done, but the constructor of `ClientPool` may change. Because the members of `reconnectExc` will be deleted. Does this affect other component calls？
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/ClientPool.java
##########
@@ -54,7 +54,7 @@
return action.run(client);
} catch (Exception exc) {
- if (reconnectExc.isInstance(exc)) {
+ if (reconnectExc.isInstance(exc) || failureDetection(exc)) {
Review comment:
Or just move reconnectExc.isInstance(exc) into the failureDetection(exc) method in `ClientPool`, like this:
```
try {
return action.run(client);
} catch (Exception exc) {
if (failureDetection(exc)) {
...
}
}
protected boolean failureDetection(Exception exc) {
return reconnectExc.isInstance(exc);
}
```
@pvary
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/ClientPool.java
##########
@@ -54,7 +54,7 @@
return action.run(client);
} catch (Exception exc) {
- if (reconnectExc.isInstance(exc)) {
+ if (reconnectExc.isInstance(exc) || failureDetection(exc)) {
Review comment:
Okay.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] marton-bod commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
marton-bod commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570954270
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,36 +19,88 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Function;
+import org.apache.hadoop.hive.metastore.api.FunctionType;
+import org.apache.hadoop.hive.metastore.api.GetAllFunctionsResponse;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.PrincipalType;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.thrift.transport.TTransportException;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
- try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
- pool.run(Object::toString);
- }
+ Mockito.doThrow(new RuntimeException()).when(clients).newClient();
+ clients.run(Object :: toString);
Review comment:
nit: whitespaces `Object::toString`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] marton-bod commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
marton-bod commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570975009
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,36 +19,88 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Function;
+import org.apache.hadoop.hive.metastore.api.FunctionType;
+import org.apache.hadoop.hive.metastore.api.GetAllFunctionsResponse;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.PrincipalType;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.thrift.transport.TTransportException;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
- try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
- pool.run(Object::toString);
- }
+ Mockito.doThrow(new RuntimeException()).when(clients).newClient();
+ clients.run(Object :: toString);
}
- private static class MockClientPool extends ClientPool<Object, Exception> {
+ @Test(expected = MetaException.class)
+ public void testGetTablesFailure() throws Exception {
Review comment:
can we name test case a bit differently to reflect the intent?
e.g. `testGetTablesFailsForNonReconnectableException`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r563551717
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +84,15 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(Exception e) {
+ if (Objects.nonNull(e) && e instanceof MetaException &&
Review comment:
nit: Do we need to use `Objects.nonNull(e)`? Maybe I am old fashioned but should `e != null` suffice?
Thanks,
Peter
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r563038095
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
@rdblue, @Jecarm: Moving forward with this string solves this specific problem. Later we can still reconsider if we find other cases.
Thanks @Jecarm for the investigations!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r563549890
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/ClientPool.java
##########
@@ -54,7 +54,7 @@
return action.run(client);
} catch (Exception exc) {
- if (reconnectExc.isInstance(exc)) {
+ if (reconnectExc.isInstance(exc) || failureDetection(exc)) {
Review comment:
I think it would be more easy to understand if we move `reconnectExc.isInstance(exc)` into the `failureDetection(exc)` method as well.
What do you think?
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +84,15 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(Exception e) {
+ if (Objects.nonNull(e) && e instanceof MetaException &&
Review comment:
nit: Do we need to use `Objects.nonNull(e)`? Maybe I am old fashioned but should `e != null` suffice?
Thanks,
Peter
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/ClientPool.java
##########
@@ -54,7 +54,7 @@
return action.run(client);
} catch (Exception exc) {
- if (reconnectExc.isInstance(exc)) {
+ if (reconnectExc.isInstance(exc) || failureDetection(exc)) {
Review comment:
@Jecarm: This is what I have been thinking about too.
And in `HiveClientPool` we can do something like this:
```
@Override
protected boolean failureDetection(Exception e) {
if (super.failureDetection(e) || (e != null && e instanceof MetaException &&
e.getMessage().contains("Got exception: org.apache.thrift.transport.TTransportException"))) {
return true;
}
return false;
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570773304
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -51,4 +81,918 @@ protected void close(Object client) {
}
}
+
+ private static class MockHiveClientPool extends ClientPool<IMetaStoreClient, Exception> {
+
+ MockHiveClientPool(int poolSize, Class<? extends Exception> reconnectExc) {
+ super(poolSize, reconnectExc);
+ }
+
+ @Override
+ protected IMetaStoreClient newClient() {
+ return new MockMetaStoreFailureClient();
Review comment:
I refactored the test unit, please review it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570854701
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
+ }
+
+ @Test(expected = MetaException.class)
+ public void testConnectionFailure() throws Exception {
Review comment:
Since we are testing that for a 'normal' MetaException we do not try to reconnect.
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,36 +19,88 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Function;
+import org.apache.hadoop.hive.metastore.api.FunctionType;
+import org.apache.hadoop.hive.metastore.api.GetAllFunctionsResponse;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.PrincipalType;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.thrift.transport.TTransportException;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
Review comment:
We usually check the exceptions this way, so we can check the exception message easily:
```
@Test
public void testNewClientFailure() {
Mockito.doThrow(new RuntimeException("Connection exception")).when(clients).newClient();
AssertHelpers.assertThrows("should throw exception", RuntimeException.class,
"Connection exception", () -> clients.run(Object :: toString)
);
}
```
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,36 +19,88 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Function;
+import org.apache.hadoop.hive.metastore.api.FunctionType;
+import org.apache.hadoop.hive.metastore.api.GetAllFunctionsResponse;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.PrincipalType;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.thrift.transport.TTransportException;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
- try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
- pool.run(Object::toString);
- }
+ Mockito.doThrow(new RuntimeException()).when(clients).newClient();
+ clients.run(Object :: toString);
}
- private static class MockClientPool extends ClientPool<Object, Exception> {
+ @Test(expected = MetaException.class)
Review comment:
Same here as above for the exception handling
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,36 +19,88 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Function;
+import org.apache.hadoop.hive.metastore.api.FunctionType;
+import org.apache.hadoop.hive.metastore.api.GetAllFunctionsResponse;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.PrincipalType;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.thrift.transport.TTransportException;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
- try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
- pool.run(Object::toString);
- }
+ Mockito.doThrow(new RuntimeException()).when(clients).newClient();
+ clients.run(Object :: toString);
}
- private static class MockClientPool extends ClientPool<Object, Exception> {
+ @Test(expected = MetaException.class)
+ public void testGetTablesFailure() throws Exception {
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+ Mockito.doThrow(new MetaException("Another meta exception"))
+ .when(hmsClient).getTables(Mockito.anyString(), Mockito.anyString());
+ clients.run(client -> client.getTables("default", "t"));
+ }
+
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
Review comment:
I think it would be easier to understand the test if we split this to 2:
- testTTransportException
- testWrappedTTransportException
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,36 +19,88 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Function;
+import org.apache.hadoop.hive.metastore.api.FunctionType;
+import org.apache.hadoop.hive.metastore.api.GetAllFunctionsResponse;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.PrincipalType;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.thrift.transport.TTransportException;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
- try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
- pool.run(Object::toString);
- }
+ Mockito.doThrow(new RuntimeException()).when(clients).newClient();
+ clients.run(Object :: toString);
}
- private static class MockClientPool extends ClientPool<Object, Exception> {
+ @Test(expected = MetaException.class)
+ public void testGetTablesFailure() throws Exception {
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+ Mockito.doThrow(new MetaException("Another meta exception"))
+ .when(hmsClient).getTables(Mockito.anyString(), Mockito.anyString());
+ clients.run(client -> client.getTables("default", "t"));
+ }
+
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
Review comment:
Or simply 2 tests which call the same method where the exception is a parameter
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r561447854
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
I might not understand the fix. From reading #1994, I thought the problem was that some client calls throw the wrong exception, a `MetaException` that wraps `TTransportException` instead of `TTransportException`. That causes the reconnect detection to fail and the `MetaException` is thrown instead.
I would then expect the fix to be to detect the `MetaException` that wraps `TTransportException` and return true because the connection should be reconnected.
The implementation here appears to be to try to use the connection again and return true if it fails with `TTransportException` the second time or false (do not reconnect) if it succeeds. But why issue the second call instead of detecting the failure case using `ex.getCause()`?
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
@pvary, what do you think?
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
Hm. What is the exception message when the connection is to blame? I don't like that solution either, but it seems better than making a second RPC call if we have something that is reliable there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r564158456
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,15 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(Exception e) {
+ if (super.failureDetection(e) || (e != null && e instanceof MetaException &&
+ e.getMessage().contains("Got exception: org.apache.thrift.transport.TTransportException"))) {
Review comment:
Nit: continuation indents are 2 indents / 4 spaces.
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,15 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(Exception e) {
Review comment:
How about renaming this to `isConnectionException`? I think that is more clear about what the returned value is than `failureDetection`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r565058304
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -51,4 +81,918 @@ protected void close(Object client) {
}
}
+
+ private static class MockHiveClientPool extends ClientPool<IMetaStoreClient, Exception> {
+
+ MockHiveClientPool(int poolSize, Class<? extends Exception> reconnectExc) {
+ super(poolSize, reconnectExc);
+ }
+
+ @Override
+ protected IMetaStoreClient newClient() {
+ return new MockMetaStoreFailureClient();
+ }
+
+ @Override
+ protected IMetaStoreClient reconnect(IMetaStoreClient client) {
+ return new MockMetaStoreSuccessClient();
+ }
+
+ @Override
+ protected void close(IMetaStoreClient client) {
+
+ }
+
+ @Override
+ protected boolean isConnectionException(Exception e) {
Review comment:
Why do we override this method?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r561447854
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
I might not understand the fix. From reading #1994, I thought the problem was that some client calls throw the wrong exception, a `MetaException` that wraps `TTransportException` instead of `TTransportException`. That causes the reconnect detection to fail and the `MetaException` is thrown instead.
I would then expect the fix to be to detect the `MetaException` that wraps `TTransportException` and return true because the connection should be reconnected.
The implementation here appears to be to try to use the connection again and return true if it fails with `TTransportException` the second time or false (do not reconnect) if it succeeds. But why issue the second call instead of detecting the failure case using `ex.getCause()`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570773304
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -51,4 +81,918 @@ protected void close(Object client) {
}
}
+
+ private static class MockHiveClientPool extends ClientPool<IMetaStoreClient, Exception> {
+
+ MockHiveClientPool(int poolSize, Class<? extends Exception> reconnectExc) {
+ super(poolSize, reconnectExc);
+ }
+
+ @Override
+ protected IMetaStoreClient newClient() {
+ return new MockMetaStoreFailureClient();
Review comment:
I refactored the test unit, please review it.
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
Review comment:
i will fix them.
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
Review comment:
the exception of `MetaException` or `TTransportException` for `getAllFunctions` in this method will not be triggered. If need test `TTransportException`, we will create a new client In a separate method.
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
Review comment:
not necessary
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
+ }
+
+ @Test(expected = MetaException.class)
+ public void testConnectionFailure() throws Exception {
Review comment:
Can we use `testConnectionFailure` test method implementation instead of `testNewClientFailure` ?
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
+ }
+
+ @Test(expected = MetaException.class)
+ public void testConnectionFailure() throws Exception {
Review comment:
like this:
@Test(expected = MetaException.class)
public void testNewClientFailure() throws Exception {
HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
Mockito.doReturn(hmsClient).when(clients).newClient();
Mockito.doThrow(new MetaException("Another meta exception"))
.when(hmsClient).getTables(Mockito.anyString(), Mockito.anyString());
clients.run(client -> client.getTables("default", "t"));
}
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
+ }
+
+ @Test(expected = MetaException.class)
+ public void testConnectionFailure() throws Exception {
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+ Mockito.doThrow(new MetaException("Another meta exception"))
+ .when(hmsClient).getTables(Mockito.anyString(), Mockito.anyString());
+ clients.run(client -> client.getTables("default", "t"));
+ }
+
private static class MockClientPool extends ClientPool<Object, Exception> {
Review comment:
I will clear it
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
+ }
+
+ @Test(expected = MetaException.class)
+ public void testConnectionFailure() throws Exception {
Review comment:
I submitted some code, Please check it out.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#issuecomment-767698123
> Could we add some tests for this code path? Maybe using some MockClients to check it the reconnect is attempted when needed?
I agree that it would be great to test this code path, but adding the mock probably isn't that straightforward. Do we have existing mock tests for this that could be adapted? If not, we may want to get it in and then follow up with tests later.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r565057472
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -51,4 +81,918 @@ protected void close(Object client) {
}
}
+
+ private static class MockHiveClientPool extends ClientPool<IMetaStoreClient, Exception> {
+
+ MockHiveClientPool(int poolSize, Class<? extends Exception> reconnectExc) {
+ super(poolSize, reconnectExc);
+ }
+
+ @Override
+ protected IMetaStoreClient newClient() {
+ return new MockMetaStoreFailureClient();
Review comment:
Instead of creating our own client we might want to use Mockito to generate an object for us. Some good examles can be found here: https://github.com/apache/iceberg/blob/915a17573f2819992c866c9a76fbbf9612ca1d4a/aws/src/test/java/org/apache/iceberg/aws/glue/GlueCatalogTest.java
We can say that if a given method is called then return the given value. We can even define that the 2nd call should return another value.
Also when checking the results we can check the number of calls for the given methods.
Thanks for creating the test case!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r567788926
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -51,4 +81,918 @@ protected void close(Object client) {
}
}
+
+ private static class MockHiveClientPool extends ClientPool<IMetaStoreClient, Exception> {
+
+ MockHiveClientPool(int poolSize, Class<? extends Exception> reconnectExc) {
+ super(poolSize, reconnectExc);
+ }
+
+ @Override
+ protected IMetaStoreClient newClient() {
+ return new MockMetaStoreFailureClient();
Review comment:
@Jecarm: I would do something like this:
```
@Override
protected HiveMetaStoreClient newClient() {
HiveMetaStoreClient client = Mockito.mock(HiveMetaStoreClient.class);
try {
Mockito.doThrow(TTransportException.class).when(client).getAllDatabases();
Mockito.doThrow(new MetaException("Got exception: org.apache.thrift.transport.TTransportException")).when(client).getAllFunctions();
Mockito.doThrow(new MetaException("Another meta exception")).when(client).getTables(Mockito.anyString(), Mockito.anyString());
} catch (Exception e) {
throw new RuntimeException("This should not happen", e);
}
return client;
}
@Override
protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
try {
Mockito.doReturn(new ArrayList<>()).when(newClient).getAllDatabases();
Mockito.doReturn(new GetAllFunctionsResponse()).when(newClient).getAllFunctions();
Mockito.doReturn(new ArrayList<>()).when(newClient).getAllTables(Mockito.anyString());
} catch (Exception e) {
throw new RuntimeException("RE", e);
}
return newClient;
}
```
After more thought I think checking the number of reconnection calls through the number of calls on the mock objects could be misleading, so I think we could check them in the `MockClientPool` in a counter / boolean if we do not find a more elegant way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r564236231
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,15 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(Exception e) {
Review comment:
Replaced the original with the new method name of `isConnectionException`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570800729
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
Review comment:
nit: unnecessary line break
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
Review comment:
nit: unnecessary line break
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
Review comment:
nit: unnecessary line break
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570253524
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -51,4 +81,918 @@ protected void close(Object client) {
}
}
+
+ private static class MockHiveClientPool extends ClientPool<IMetaStoreClient, Exception> {
+
+ MockHiveClientPool(int poolSize, Class<? extends Exception> reconnectExc) {
+ super(poolSize, reconnectExc);
+ }
+
+ @Override
+ protected IMetaStoreClient newClient() {
+ return new MockMetaStoreFailureClient();
Review comment:
Even better, we do not have to create a new class for the tests, but we can use `Mockito.spy()` on `HiveClientPool` and return the different clients on `newClient` and `reconnect`
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
Review comment:
nit: Please import concrete classes
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
Review comment:
nit: unnecessary line break
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
Review comment:
nit: unnecessary line break
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
Review comment:
nit: unnecessary line break
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
Review comment:
I think it is perfectly ok to test only with a single method with one type of exception.
Do we want to test failures with `new TTransportException` as well?
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
Review comment:
Should we check?
```
Mockito.verify(clients).reconnect(hmsClient);
Mockito.verify(clients, never()).reconnect(hmsClient);
```
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
+ }
+
+ @Test(expected = MetaException.class)
+ public void testConnectionFailure() throws Exception {
Review comment:
If I understand correctly this test the name is slightly misleading.
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
+ }
+
+ @Test(expected = MetaException.class)
+ public void testConnectionFailure() throws Exception {
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+ Mockito.doThrow(new MetaException("Another meta exception"))
+ .when(hmsClient).getTables(Mockito.anyString(), Mockito.anyString());
+ clients.run(client -> client.getTables("default", "t"));
+ }
+
private static class MockClientPool extends ClientPool<Object, Exception> {
Review comment:
Do we still need this class?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570253524
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -51,4 +81,918 @@ protected void close(Object client) {
}
}
+
+ private static class MockHiveClientPool extends ClientPool<IMetaStoreClient, Exception> {
+
+ MockHiveClientPool(int poolSize, Class<? extends Exception> reconnectExc) {
+ super(poolSize, reconnectExc);
+ }
+
+ @Override
+ protected IMetaStoreClient newClient() {
+ return new MockMetaStoreFailureClient();
Review comment:
Even better, we do not have to create a new class for the tests, but we can use `Mockito.spy()` on `HiveClientPool` and return the different clients on `newClient` and `reconnect`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r571943016
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
+ }
+
+ @Test(expected = MetaException.class)
+ public void testConnectionFailure() throws Exception {
Review comment:
Looks good. Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570819489
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
+ }
+
+ @Test(expected = MetaException.class)
+ public void testConnectionFailure() throws Exception {
Review comment:
If I understand correctly this test the name is slightly misleading.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] marton-bod commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
marton-bod commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570982789
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,36 +19,88 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Function;
+import org.apache.hadoop.hive.metastore.api.FunctionType;
+import org.apache.hadoop.hive.metastore.api.GetAllFunctionsResponse;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.PrincipalType;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.thrift.transport.TTransportException;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
- try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
- pool.run(Object::toString);
- }
+ Mockito.doThrow(new RuntimeException()).when(clients).newClient();
+ clients.run(Object :: toString);
}
- private static class MockClientPool extends ClientPool<Object, Exception> {
+ @Test(expected = MetaException.class)
+ public void testGetTablesFailure() throws Exception {
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+ Mockito.doThrow(new MetaException("Another meta exception"))
+ .when(hmsClient).getTables(Mockito.anyString(), Mockito.anyString());
+ clients.run(client -> client.getTables("default", "t"));
+ }
+
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
Review comment:
can we extract this List creation so we can reuse it two lines below?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r561690101
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
MetaException does not have an embedded Exception (no `ex.getCause()` because the MetaException is declared on the Thrift interface and only has a message field) 😢. Also it is used for signaling connection problems and input data issues as well, and the only difference between those is the exception message text. So we can only check the message text to decide on the cause which is too brittle for my taste.
So I would prefer a solution where we do not try to differentiate between the MetaExceptions parsing the text message.
Since we can not have a definite answer when the error is caused by a connection problem or a bad parametrization of the HMS call, I think reconnection every time when we have a MetaException should be avoided if possible. I would use a cheap probe to check the liveliness of the connection (for example `client.getServerVersion()`)
I see the following possibilities:
- Periodic check for the connection - Fix number of extra HMS calls, no extra delay on other HMS calls
- Check when there is a MetaException - Double HMS calls for every call where the response is MetaException
- Some combination of the above
The decision should depend on the use-cases we would like to cover.
- Who will use the `ClientPool`?
- How likely that we issue calls which are badly parametrized?
- How likely that we will have connection issues?
To answer the questions from the Hive side:
- I do not see places where we can have MetaException because of the wrong parametrization of the calls (if so we have to fix it 😄)
- If there is a MetaException then that is because of the connection issues
OTOH I consider ClientPool as an API which could/should be used by other components, and such an API should handle errors better. I would accept the extra complexity to avoid extra `reconnect` attempts, and even maybe extra HMS calls too, but can be convinced otherwise.
That's my 2 cents.
Peter
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570820895
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -19,17 +19,83 @@
package org.apache.iceberg.hive;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.mockito.Mockito;
public class TestClientPool {
+ HiveClientPool clients;
+
+ @Before
+ public void before() {
+ HiveClientPool clientPool = new HiveClientPool(2, new Configuration());
+ clients = Mockito.spy(clientPool);
+ }
+
+ @After
+ public void after() {
+ clients.close();
+ clients = null;
+ }
+
@Test(expected = RuntimeException.class)
public void testNewClientFailure() throws Exception {
try (MockClientPool pool = new MockClientPool(2, Exception.class)) {
pool.run(Object::toString);
}
}
+ @Test
+ public void testConnectionFailureRestore() throws Exception {
+
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+
+ // Throwing an exception may trigger the client to reconnect.
+ String metaMessage = "Got exception: org.apache.thrift.transport.TTransportException";
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllDatabases();
+ Mockito.doThrow(new MetaException(metaMessage)).when(hmsClient).getAllFunctions();
+
+ // Create a new client when the reconnect method is called.
+ HiveMetaStoreClient newClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(newClient).when(clients).reconnect(hmsClient);
+
+ Mockito.doReturn(Lists.newArrayList("db1", "db2")).when(newClient).getAllDatabases();
+ GetAllFunctionsResponse response = new GetAllFunctionsResponse();
+ response.addToFunctions(
+ new Function("concat", "db1", "classname", "root", PrincipalType.USER, 100, FunctionType.JAVA, null));
+ Mockito.doReturn(response)
+ .when(newClient).getAllFunctions();
+ // The return is OK when the reconnect method is called.
+ Assert.assertEquals(Lists.newArrayList("db1", "db2"),
+ clients.run(client -> client.getAllDatabases()));
+
+ //assert && verify
+ Mockito.verify(clients).reconnect(hmsClient);
+
+ // The return is OK, because the client pool uses a new client
+ Assert.assertEquals(response,
+ clients.run(client -> client.getAllFunctions()));
+
+ Mockito.verify(clients, Mockito.times(1)).reconnect(hmsClient);
+ }
+
+ @Test(expected = MetaException.class)
+ public void testConnectionFailure() throws Exception {
+ HiveMetaStoreClient hmsClient = Mockito.mock(HiveMetaStoreClient.class);
+ Mockito.doReturn(hmsClient).when(clients).newClient();
+ Mockito.doThrow(new MetaException("Another meta exception"))
+ .when(hmsClient).getTables(Mockito.anyString(), Mockito.anyString());
+ clients.run(client -> client.getTables("default", "t"));
+ }
+
private static class MockClientPool extends ClientPool<Object, Exception> {
Review comment:
Do we still need this class?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r567788926
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestClientPool.java
##########
@@ -51,4 +81,918 @@ protected void close(Object client) {
}
}
+
+ private static class MockHiveClientPool extends ClientPool<IMetaStoreClient, Exception> {
+
+ MockHiveClientPool(int poolSize, Class<? extends Exception> reconnectExc) {
+ super(poolSize, reconnectExc);
+ }
+
+ @Override
+ protected IMetaStoreClient newClient() {
+ return new MockMetaStoreFailureClient();
Review comment:
@Jecarm: I would do something like this:
```
@Override
protected IMetaStoreClient newClient() {
IMetaStoreClient client = Mockito.mock(IMetaStoreClient.class);
try {
Mockito.doThrow(TTransportException.class).when(client).getAllDatabases();
Mockito.doThrow(new MetaException("Got exception: org.apache.thrift.transport.TTransportException")).when(client).getAllFunctions();
Mockito.doThrow(new MetaException("Another meta exception")).when(client).getTables(Mockito.anyString(), Mockito.anyString());
} catch (Exception e) {
throw new RuntimeException("This should not happen", e);
}
return client;
}
@Override
protected IMetaStoreClient reconnect(IMetaStoreClient client) {
IMetaStoreClient newClient = Mockito.mock(IMetaStoreClient.class);
try {
Mockito.doReturn(new ArrayList<>()).when(newClient).getAllDatabases();
Mockito.doReturn(new GetAllFunctionsResponse()).when(newClient).getAllFunctions();
Mockito.doReturn(new ArrayList<>()).when(newClient).getAllTables(Mockito.anyString());
} catch (Exception e) {
throw new RuntimeException("RE", e);
}
return newClient;
}
```
After more thought I think checking the number of reconnection calls through the number of calls on the mock objects could be misleading, so I think we could check them in the `MockClientPool` in a counter / boolean if we do not find a more elegant way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] marton-bod commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
marton-bod commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r570953851
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,15 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean isConnectionException(Exception e) {
+ if (super.isConnectionException(e) || (e != null && e instanceof MetaException &&
+ e.getMessage().contains("Got exception: org.apache.thrift.transport.TTransportException"))) {
Review comment:
nit: can we just say:
```
return super.isConnectionException(e) || (e != null && e instanceof MetaException &&
e.getMessage().contains("Got exception: org.apache.thrift.transport.TTransportException"));
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Jecarm commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
Jecarm commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r571352094
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,15 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean isConnectionException(Exception e) {
+ if (super.isConnectionException(e) || (e != null && e instanceof MetaException &&
+ e.getMessage().contains("Got exception: org.apache.thrift.transport.TTransportException"))) {
Review comment:
May lead to poor debugging
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r563549890
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/ClientPool.java
##########
@@ -54,7 +54,7 @@
return action.run(client);
} catch (Exception exc) {
- if (reconnectExc.isInstance(exc)) {
+ if (reconnectExc.isInstance(exc) || failureDetection(exc)) {
Review comment:
I think it would be more easy to understand if we move `reconnectExc.isInstance(exc)` into the `failureDetection(exc)` method as well.
What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2119:
URL: https://github.com/apache/iceberg/pull/2119#discussion_r562285899
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -83,6 +83,20 @@ protected HiveMetaStoreClient reconnect(HiveMetaStoreClient client) {
return client;
}
+ @Override
+ protected boolean failureDetection(HiveMetaStoreClient client, Exception ex) {
Review comment:
Hm. What is the exception message when the connection is to blame? I don't like that solution either, but it seems better than making a second RPC call if we have something that is reliable there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org