Alex Behm has posted comments on this change. Change subject: IMPALA-4789: Fix slow metadata loading due to inconsistent paths. ......................................................................
Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/5743/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: Line 275: // a descendant of dirPath. > do we already have test cases that will take this path? This is the 'default' path for loading the metadata of a partitioned table, so almost all tables load through this path. The above path is covered by things like INSERT, ALTER TABLE ADD PARTITION, etc. so in terms of code coverage we have everything. Line 693: tblLocation = tblLocation.makeQualified(fs.getUri(), tblLocation); > here and elsewhere: should we use createFullyQualifiedPath() to avoid the b Yes. Didn't know about that. Done. http://gerrit.cloudera.org:8080/#/c/5743/3/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java: PS3, Line 440: != > nit: I feel an XOR makes the intention more clear than using a !=, especial Rewrote this with a helper function to simplify. PS3, Line 442: pUri.getScheme().equals(parentUri.getScheme()) > An example that could hit NPE. isDescendantPath(new Path("/foo/bar"), new P You are right. Rewrote this with a helper function to simplify. -- To view, visit http://gerrit.cloudera.org:8080/5743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8c881b7cb155032b82fba0e29350ca31de388d55 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
