okumin commented on PR #4444: URL: https://github.com/apache/hive/pull/4444#issuecomment-1611757065
> @okumin Could you please address the comment in [the original pull request](https://github.com/apache/hive/pull/1402)? The limitation with this approach is we don't get the features implemented in SessionHiveMetastoreClient (Example: handling of temp tables). This will lead to the issue of temp tables not getting cleaned up when the session is closed. Please see the comments from @thejasmn and @alanfgates on [HIVE-12679](https://issues.apache.org/jira/browse/HIVE-12679). @ganeshashree Quickly checking SessionHiveMetastoreClient, it has the following additional features. - Temporary table management in most methods - Query cache management - TX id management It sounds like a good idea to make it easy to compose the features with a custom metastore client. I am personally thinking we can work on it in another ticket and I will definitely try it. In my mind, I wonder how about the following I/F. - `IMetastoreClient` - It is the interface of any metastore client. - `HiveMetaStoreClient` - It is the implementation of `IMetastoreClient` to talk to Hive Metastore - `SessionHiveMetaStoreClient` - It is a proxy to add the above three features to any `IMetastoreClient` - `RetryingMetaStoreClient` - It is a dynamic proxy to add retry capability to an `IMetastoreClient` relying on `HiveMetastoreClient` As proposed in the ticket, I expect we can implement `SessionHiveMetaStoreClient` by composition rather than inheritance like below. ``` class SessionHiveMetaStoreClient implement IMetastoreClient { private IMetastoreClient underlying; @Override protected void create_table(CreateTableRequest request) throws InvalidObjectException, MetaException, NoSuchObjectException, TException { org.apache.hadoop.hive.metastore.api.Table tbl = request.getTable(); if (tbl.isTemporary()) { createTempTable(tbl); return; } underlying.create_table(request); } } ``` If we follow the current API design of `HiveMetaStoreClientFactory`, its responsibility will be just "create an IMetastoreClient". Users may wrap their custom clients with `SessionHiveMetaStoreClient` if they need to support temp tables, or may wrap them with `RetryingMetaStoreClient` if the custom clients depend on Thrift. It is up to the users. I guess it would give us the best flexibility(I guess all the past authors have not needed features of SessionHiveMetaStoreClient so far and it could be possible that someone will want to customize such session-related features for some reason of their platforms). @wecharyu and @deniskuzZ have different opinions about how to generate an instance. So, the entire architecture has not decided yet, though. -- 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]
