szehon-ho commented on a change in pull request #3099:
URL: https://github.com/apache/iceberg/pull/3099#discussion_r715796353
##########
File path:
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveClientPool.java
##########
@@ -110,57 +103,4 @@ public void
testGetTablesFailsForNonReconnectableException() throws Exception {
AssertHelpers.assertThrows("Should throw exception", MetaException.class,
"Another meta exception", () -> clients.run(client ->
client.getTables("default", "t")));
}
-
- @Test
- public void testConnectionFailureRestoreForMetaException() throws Exception {
Review comment:
Good idea, done. Put the test back.
Side note: the flexibility of making it configurable is nice, and is a
potential soution the issue I mentioned, I am tempted to set it to true in
HiveClientOperations::acquireLock in this line to address the issue I mentioned
above.
```
LockResponse lockResponse = metaClients.run(client ->
client.lock(lockRequest));
```
I can see from Hive point of view why they don't want to auto-retry, but
most exception scenarios are not going to be in the corner case of having
succeeded in the backend and leaving the lock. In all honesty, if the Iceberg
job fails because lock() throws TTransportException, end user will just try
again but it is a wasted effort. Not sure if any thoughts on it?
cc @pvary
##########
File path:
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -21,22 +21,24 @@
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hive.conf.HiveConf;
-import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.IMetaStoreClient;
+import org.apache.hadoop.hive.metastore.RetryingMetaStoreClient;
import org.apache.hadoop.hive.metastore.api.MetaException;
import org.apache.iceberg.ClientPoolImpl;
-import org.apache.iceberg.common.DynConstructors;
+import org.apache.iceberg.common.DynMethods;
import
org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
import org.apache.thrift.TException;
import org.apache.thrift.transport.TTransportException;
-public class HiveClientPool extends ClientPoolImpl<HiveMetaStoreClient,
TException> {
+public class HiveClientPool extends ClientPoolImpl<IMetaStoreClient,
TException> {
- // use appropriate ctor depending on whether we're working with Hive2 or
Hive3 dependencies
- // we need to do this because there is a breaking API change between Hive2
and Hive3
- private static final DynConstructors.Ctor<HiveMetaStoreClient> CLIENT_CTOR =
DynConstructors.builder()
- .impl(HiveMetaStoreClient.class, HiveConf.class)
- .impl(HiveMetaStoreClient.class, Configuration.class)
- .build();
+ // use appropriate ctor depending on whether we're working with Hive1,
Hive2, or Hive3 dependencies
+ // we need to do this because there is a breaking API change between Hive1,
Hive2, and Hive3
+ private static final DynMethods.StaticMethod CLIENT_CTOR =
DynMethods.builder("getProxy")
Review comment:
Done
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
