Alex Behm has posted comments on this change. Change subject: IMPALA-5431: Remove redundant path exists checks during table load ......................................................................
Patch Set 4: Code-Review+1 (7 comments) http://gerrit.cloudera.org:8080/#/c/7095/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: Line 267: // No need to load blocks for empty partitions list. > Okay. I felt this adds a lot of try{} catch{} to the code and hence didn't Right, but saving an RPC seems worth it. Line 725: } remove while you are here http://gerrit.cloudera.org:8080/#/c/7095/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: Line 266: FileSystem fs = dirPath.getFileSystem(CONF); Move this closer to where it is used. getFileSystem() tends to be expensive, so great if we can avoid it! Line 305: if (fileStatusIter == null) { I think you can make this a single line and remove the comment. Line 371: if (fileStatusIter == null) { I think you can make this a single line and remove the comment. Line 1709: if (statuses == null) { I think you can make this a single line and remove the comment. http://gerrit.cloudera.org:8080/#/c/7095/4/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java: Line 491: * the path does not exist. This helps simplifies the caller code in cases where This helps simplify the caller code... -- To view, visit http://gerrit.cloudera.org:8080/7095 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id10ecf64ea2eda2d0f9299c0aa371933eca22281 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
