Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10587 )
Change subject: IMPALA-6994:Avoid reloading a table's HMS data for file-only operations ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/10587/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/10587/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1365 PS2, Line 1365: loadMetadataAndDiskIds(partitionsToUpdateFileMdByPath, true); any way to refactor this function so you don't need to duplicate this code from line 1429-1435 up to here? perhaps extracting another function for all of the partition-reloading code and only call it when you dont have the 'no new partitions' hint? http://gerrit.cloudera.org:8080/#/c/10587/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/10587/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@640 PS2, Line 640: private void loadTableMetadata(Table tbl, long newCatalogVersion, the arguments here are getting quite unwieldly, and it seems that the two existing booleans have a positive sense ("DO reload filemetadata" and "DO reload table schema") whereas this new boolean has a negative sense "DONT reload data from HMS". How about refactoring this slightly to use an enum and an EnumSet, something like: static enum MetadataLoadFlag { RELOAD_TABLE_METADATA, RELOAD_PARTITION_METADATA, RELOAD_FILE_METADATA } void loadTableMetadata(Table tbl, long newCatalogVersion, EnumSet<MetadataLoadFlag> flags, Set<String> partitionsToUpdate, Table msTable); .. and perhaps add Preconditions that validate that the optional msTable, partitionsToUpdate, etc params are only set when it makes sense according to the flags? I think this would make the call sites and code a lot more understandable. -- To view, visit http://gerrit.cloudera.org:8080/10587 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I331bb0371fde287f43a85b025b4f98cb45f3eb3c Gerrit-Change-Number: 10587 Gerrit-PatchSet: 2 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Fri, 06 Jul 2018 17:54:11 +0000 Gerrit-HasComments: Yes
