Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-5431: Remove redundant path exists check during table load ......................................................................
Patch Set 2: (5 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: if (partsByPath.size() == 0 || !fs.exists(dirPath)) return; > Is this exists() really needed? We are going to do a listFiles() which thro Okay. I felt this adds a lot of try{} catch{} to the code and hence didn't do it. But if it saves an RPC, why not :) Line 647: if (fs.exists(tblLocation)) { > We check exists() inside getAvailableAccessLevel() I thought about this, but didn't want to do it because of the following scenario. If the table directory doesn't exist, getAvailableAccessLevel() recurses up to the parent path, and if the parent had a READ only permission for example, that would make the table READ_ONLY. Seemed like a regression in behavior if a user wants to insert data into it. Not totally sure if such a situation is possible. Line 707: if (!fs.exists(partDir)) { > listStatus() throws if the path does not exist, in that sense this check se Done Line 1108: if (fs.exists(location)) { > exists() is checked inside getAvailableAccessLevel() Same as above, please let me know if I'm wrong. Line 1662: if (!fs.exists(path)) return; > Is this really necessary? Whatever we do inside getAllPartitionsNotInHms() Done -- 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: 2 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
