okumin commented on code in PR #4444:
URL: https://github.com/apache/hive/pull/4444#discussion_r1238528174
##########
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java:
##########
@@ -5700,11 +5701,26 @@ public HiveMetaHook getHook(
}
};
- if (conf.getBoolVar(ConfVars.METASTORE_FASTPATH)) {
- return new SessionHiveMetaStoreClient(conf, hookLoader, allowEmbedded);
- } else {
- return RetryingMetaStoreClient.getProxy(conf, hookLoader,
metaCallTimeMap,
- SessionHiveMetaStoreClient.class.getName(), allowEmbedded);
+ return createMetaStoreClientFactory(conf)
+ .createMetaStoreClient(conf, hookLoader, allowEmbedded,
metaCallTimeMap);
+ }
+
+ private static HiveMetaStoreClientFactory
createMetaStoreClientFactory(HiveConf conf) throws
+ MetaException {
+ String metaStoreClientFactoryClassName = MetastoreConf.getVar(conf,
+ MetastoreConf.ConfVars.METASTORE_CLIENT_FACTORY_CLASS);
Review Comment:
@deniskuzZ @wecharyu Thanks for your opinions. Let me clarify your
suggestions first. This is my understanding.
- `IHMSHandler`: The essential interface. `HMSHandler` is the primary and
only implementation for now
- `AbstractHMSHandlerProxy`: The interface of dynamic proxy for
`IHMHandler`. `RetryingHMSHandler` is the primary implementation and it becomes
configurable thanks to #4257
- `HMSHandlerProxyFactory`: It has one static method,
`getProxy(Configuration, IMSHandler, boolean)`, to wrap the given `IHMSHandler`
with the configured `AbstractHMSHandlerProxy`
I would also say the relation between `IHMSHandler` and `RetryingHMSHandler`
is similar to between `IMetaStoreClient` and `RetryingMetaStoreClient`. But the
purpose is a bit different. We'd like to make pluggable not
`RetryingMetaStoreClient` but `IMetaStoreClient` here. Also, it is not evident
that a custom `IMetaStoreClient` needs a dynamic proxy(e.g. I think at least
the [one for AWS Data
Catelog](https://github.com/awslabs/aws-glue-data-catalog-client-for-apache-hive-metastore/blob/branch-3.4.0/aws-glue-datacatalog-hive3-client/src/main/java/com/amazonaws/glue/catalog/metastore/AWSCatalogMetastoreClient.java)
may not need RetryingMetaStoreClient or another proxy because AWS SDK can
provide more purpose-built helpers).
So, if we want to satisfy our requirements, we have to make both
`IMetaStoreClient` and the proxy(e.g. NopProxy for AWS Glue Catalog)
configurable. I think it is acceptable but I wonder if we should configure two
parameters, maybe `metastore.client.class` and `metastore.client.proxy.class`,
in order to generate one `IMetaStoreClient`.
As to another aspect, the current patch and
`hive.metastore.client.factory.class` are already and unfortunately accepted by
several services... Looks like it is used in AWS Glue, [Amazon
EMR](https://docs.aws.amazon.com/emr/latest/ReleaseGuide/emr-480-release.html),
and [Databricks](https://docs.databricks.com/release-notes/runtime/5.5.html).
My company is also using it. We might not need to take care of those ones too
much, but they and their users might be a little confused.
I don't have strong opinions, and to be honest, I could be misunderstanding
your suggestion. Please feel free to give me your thoughts. I am willing to
follow that advice!
--
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]
