Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17298 )
Change subject: IMPALA-10648: Invalidate catalogd table cache for hms ddl apis which modify tables and partitions. ...................................................................... Patch Set 3: (13 comments) The solution looks good to me. Please adjust the code style to match existing codes. Thanks! http://gerrit.cloudera.org:8080/#/c/17298/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17298/3//COMMIT_MSG@9 PS3, Line 9: For non transactional tables Could you explain why we don't do this for transactional tables in the commit message? http://gerrit.cloudera.org:8080/#/c/17298/3//COMMIT_MSG@13 PS3, Line 13: (since table loading in cache takes time) but ensures consistency. This change is behind catalogd nit: please adjust the commit message body to fit into at-most 72 chars per line. http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java File fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java: http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@141 PS3, Line 141: import org.apache.hadoop.hive.metastore.api.InvalidPartitionException; Is this used somewhere? http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@222 PS3, Line 222: UnknownPartitionException Is this used somewhere? http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@347 PS3, Line 347: nit: We use 4 spaces indention for multi-line statement. However, many changes in this patch use 8 spaces. Could you configure your IDE to adjust this? http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@349 PS3, Line 349: nit: 4 spaces indention http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@915 PS3, Line 915: partitionList Is it possible that these partitions belong to different tables? * If no, let's call invalidateNonTransactionalTableIfExists() once * If yes, let's de-duplicate the table names first and then call invalidateNonTransactionalTableIfExists() for each table. http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@929 PS3, Line 929: for (PartitionSpec partitionSpec : list) { Same question as above http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1086 PS3, Line 1086: nit: 4 spaces indention http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1087 PS3, Line 1087: , nit: 8 spaces indention and move the comma above http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@2849 PS3, Line 2849: if nit: In our code style, we need one space after "if". Many changes in this patch don't have it. Please adjust them. Thanks! http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@2903 PS3, Line 2903: // return immediately if flag invalidateCacheOnDDLs_ is false : if(!invalidateCacheOnDDLs_) { : LOG.debug("Not removing table {}.{} from catalogd cache because " + : "invalidateCacheOnDDLs_ flag is set to {} ", dbNameWithCatalog, : tableName, invalidateCacheOnDDLs_); : return; : } : // Parse db name. Throw error if parsing fails. : String dbName = dbNameWithCatalog; : try { : dbName = MetaStoreUtils.parseDbName(dbNameWithCatalog, serverConf_)[1]; : } catch (MetaException ex) { : LOG.error("Successfully executed HMS api: {} but encountered error " + : "when parsing dbName {} to invalidate/remove table from cache with error message: {}", : apiName, dbNameWithCatalog, ex.getMessage()); : throw ex; : } : Db db = catalog_.getDb(dbName); : if(db == null) { : LOG.debug("Not removing table {}.{} because db {} does not exist in catalogd cache", : dbName, tableName, dbName); : return; : } : if(!db.containsTable(tableName)) { : LOG.debug("Not removing table {}.{} because it does not exist in catalogd's db cache", : dbName, tableName); : return; : } : org.apache.impala.catalog.Table catalogTbl = db.getTable(tableName); : Map<String, String> tblProperties = catalogTbl.getMetaStoreTable().getParameters(); : if(tblProperties == null || MetaStoreUtils.isTransactionalTable(tblProperties)) { : LOG.debug("Table {} is transactional. Not removing it from catalogd cache", catalogTbl.getFullName()); : return; : } These codes are similar to those in the above method. Can we extract them into a method? http://gerrit.cloudera.org:8080/#/c/17298/3/tests/custom_cluster/test_metastore_service.py File tests/custom_cluster/test_metastore_service.py: http://gerrit.cloudera.org:8080/#/c/17298/3/tests/custom_cluster/test_metastore_service.py@424 PS3, Line 424: nit: remove this blank line -- To view, visit http://gerrit.cloudera.org:8080/17298 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idb9cc22ebfb51948433e4d57f4705ce201acaf98 Gerrit-Change-Number: 17298 Gerrit-PatchSet: 3 Gerrit-Owner: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Wed, 14 Apr 2021 06:36:36 +0000 Gerrit-HasComments: Yes
