jcamachor commented on a change in pull request #1390:
URL: https://github.com/apache/hive/pull/1390#discussion_r468729267
##########
File path:
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
##########
@@ -1056,7 +1056,7 @@ public AggrStats getAggrColStatsFor(String catName,
String dbName, String tblNam
req.setCatName(catName);
req.setValidWriteIdList(writeIdList);
- return client.get_aggr_stats_for(req);
+ return getAggrStatsFromLocalCache(req);
Review comment:
`HiveMetaStoreClient` should not refer to local cache. Please change the
name for these methods, e.g., `getAggrStatsInternal` or anything alike.
##########
File path:
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClientWithLocalCache.java
##########
@@ -252,18 +264,86 @@ protected PartitionsSpecByExprResult
getPartitionsSpecByExprResult(PartitionsByE
return r;
}
+ @Override
+ protected Table getTableFromLocalCache(GetTableRequest req) throws
TException{
+ Table t;
+
+ if (isCacheEnabledAndInitialized() && isRequestCacheable(req,
KeyType.GET_TABLE)) {
+ CacheKey cacheKey = new CacheKey(KeyType.GET_TABLE, req);
+ try {
+ t = (Table) mscLocalCache.get(cacheKey, this::load);
+
+ if (LOG.isDebugEnabled() && RECORD_STATS) {
+ LOG.debug(cacheObjName + ": " + mscLocalCache.stats().toString());
+ }
+ } catch (UncheckedCacheException e) {
+ if (e.getCause() instanceof MetaException) {
+ throw (MetaException) e.getCause();
+ } else if (e.getCause() instanceof TException) {
+ throw (TException) e.getCause();
+ } else {
+ throw new TException(e.getCause());
+ }
+ }
+ } else {
+ t = super.getTableFromLocalCache(req);
+ }
+
+ return t;
+ }
+
+ @Override
+ protected AggrStats getAggrStatsFromLocalCache(PartitionsStatsRequest req)
throws TException {
+ AggrStats r;
+
+ if (isCacheEnabledAndInitialized() && isRequestCacheable(req,
KeyType.AGGR_COL_STATS)) {
+ CacheKey cacheKey = new CacheKey(KeyType.AGGR_COL_STATS, req);
+ try {
+ r = (AggrStats) mscLocalCache.get(cacheKey, this::load);
+
+ if (LOG.isDebugEnabled() && RECORD_STATS) {
+ LOG.debug(cacheObjName + ": " + mscLocalCache.stats().toString());
+ }
+ } catch (UncheckedCacheException e) {
+ if (e.getCause() instanceof MetaException) {
+ throw (MetaException) e.getCause();
+ } else if (e.getCause() instanceof TException) {
+ throw (TException) e.getCause();
+ } else {
+ throw new TException(e.getCause());
+ }
+ }
+ } else {
+ r = super.getAggrStatsFromLocalCache(req);
+ }
+
+ return r;
+ }
+
/**
* This method determines if the request should be cached.
* @param request Request object
* @return boolean
*/
- private boolean isRequestCachable(Object request, KeyType keyType) {
+ private boolean isRequestCacheable(Object request, KeyType keyType) {
switch (keyType) {
//cache only requests for transactional tables, with a valid table id
case PARTITIONS_BY_EXPR:
case PARTITIONS_SPEC_BY_EXPR:
PartitionsByExprRequest req = (PartitionsByExprRequest) request;
return req.getValidWriteIdList() != null && req.getId() != -1;
+ case GET_TABLE:
+ GetTableRequest gtr = (GetTableRequest) request;
+ return gtr.getValidWriteIdList() != null;
+ case AGGR_COL_STATS:
+ PartitionsStatsRequest psr = (PartitionsStatsRequest) request;
+ Table tbl = null;
+ try {
+ tbl = getTable(psr.getDbName(), psr.getTblName());
+ } catch (TException e) {
+ return false;
+ }
+ return tbl != null && tbl.getId() != -1 && psr.getValidWriteIdList()
!= null;
Review comment:
It seems `getValidWriteIdList` has a different purpose in these requests
that has to do with stats retrieval.
I think it's easier to move away from relying on it at this stage. Instead,
we could retrieve the `writeIdList` using `AcidUtils.TableSnapshot
tableSnapshot = AcidUtils.getTableSnapshot(conf, tbl);`, and create an object
in the cache that stores the `writeIdList` and the `PartitionsStatsRequest`
separately.
##########
File path:
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
##########
@@ -2255,7 +2259,7 @@ public Table getTable(String catName, String dbName,
String tableName,
if (processorIdentifier != null)
req.setProcessorIdentifier(processorIdentifier);
- Table t = client.get_table_req(req).getTable();
+ Table t = getTableFromLocalCache(req);
Review comment:
Same comment as above for `getTableFromLocalCache`.
----------------------------------------------------------------
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]
