Marcel Kornacker has posted comments on this change. Change subject: IMPALA-2518: DROP DATABASE CASCADE does not remove cache directives of tables ......................................................................
Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/1966/6/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: Line 1092: * internal cache. Re-throws any Hive Meta Store exceptions encountered during update comment to reflect additional behavior Line 1125: for(String tableName : removedDb.getAllTableNames()) { formatting Line 1183: * Does not update partition metadata! but updates table md? Line 1188: if (hdfsTable.isMarkedCached()) { is it possible for this to be set and #clustering-cols to be > 0? if not, add checkState()s Line 1190: HdfsCachingUtil.uncacheTbl(table.getMetaStoreTable()); the function comment for uncacheTbl() says not to call this for partitioned tables, but you don't check here whether it's partitioned. looking at the code, i understand that it doesn't matter, but that's not clear from just this function. you should clean this up. Line 1201: LOG.error("Unable to uncache partition: " + single line? http://gerrit.cloudera.org:8080/#/c/1966/6/fe/src/main/java/com/cloudera/impala/util/HdfsCachingUtil.java File fe/src/main/java/com/cloudera/impala/util/HdfsCachingUtil.java: Line 127: * note: If table has partitions, call uncachePartition() instead of uncacheTbl() if that's a prerequisite, you should have a checkState() in here. i think what you mean is that it does nothing if it's a partitioned table, which you should point out here. i want to understand what a function does w/o having to look at the implementation. we outlined coding standards for impala in this document, please take a look: https://docs.google.com/document/d/1TOdgYfXzPkeQ4REvejMhcLYyMFW2v0vb0v80wR87ylg/edit#heading=h.78exweq036lx Line 129: public static void uncacheTbl(org.apache.hadoop.hive.metastore.api.Table table) the function name isn't good: you're not uncaching a table, you're simply removing table-level caching directives (if you were uncaching a table, you'd also remove the partition-level directives). Line 134: if (id == null) return; are you saying that if it's partitioned, there won't be a caching directive on the table itself yand you'll simply return here? -- To view, visit http://gerrit.cloudera.org:8080/1966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I58270e1be49e71a08e12021e7dddab01969d1810 Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Marcell Szabo <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Marcell Szabo <[email protected]> Gerrit-HasComments: Yes
