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:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to