[GitHub] [iceberg] pvary commented on a change in pull request #2119: Hive: Fix connection pool fails to connect to the Hive Metastore #1994

[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