Henry Robinson has posted comments on this change. Change subject: IMPALA-3499: Split catalog update ......................................................................
Patch Set 17: (3 comments) http://gerrit.cloudera.org:8080/#/c/3067/17/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS17, Line 1255: if (len > 100 * 1024 * 1024 /* 100MB */) { : LOG(WARNING) << "Received large catalog update(>100mb): " : << item.key << " is " : << PrettyPrinter::Print(len, TUnit::BYTES); : } > Bharath said this is for supportability so we can take action on logs. I ju Bharath, can you help me understand how this is actionable (either for supporters or users)? PS17, Line 1273: // Refresh the lib cache entries of any added functions and data sources : if (catalog_object.type == TCatalogObjectType::FUNCTION) { : DCHECK(catalog_object.__isset.fn); : LibCache::instance()->SetNeedsRefresh(catalog_object.fn.hdfs_location); : } : if (catalog_object.type == TCatalogObjectType::DATA_SOURCE) { : DCHECK(catalog_object.__isset.data_source); : LibCache::instance()->SetNeedsRefresh(catalog_object.data_source.hdfs_location); : } > Could you elaborate? what exactly is left to be done in TODO? The frontend should return the list of functions and data sources in an update from UpdateCatalogCache(), so that there's no need to deserialise the updates here. PS17, Line 1292: incremental_request > I think it is better leave it this way. I think of the 1:1 correspondence b I would prefer you changed it, please. The smaller scope that you can constrain a variable's use to, the easier it is to reason about. For example, if you create a new variable, it is immediately very clear by its definition that it points to a new TUpdateCatalogCacheRequest. When I read the patch, I had to look through to understand what incremental_request referred to by the time it got to line 1311. -- To view, visit http://gerrit.cloudera.org:8080/3067 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I176db25124a32944f2396ce8aafbed49cac95928 Gerrit-PatchSet: 17 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-HasComments: Yes
