Sailesh Mukil has posted comments on this change. Change subject: IMPALA-1878: Support INSERT and LOAD DATA on S3 and between filesystems ......................................................................
Patch Set 14: (15 comments) http://gerrit.cloudera.org:8080/#/c/2574/14/be/src/util/hdfs-bulk-ops.h File be/src/util/hdfs-bulk-ops.h: Line 64: /// Records an error if it happens during an operation. : void AddError(const string& error_msg) const; > Does this need to be public? No, I've made it private. Line 95: HdfsOperationSet(HdfsFsCache::HdfsFsMap* connection_cache); > Re-instate some comment about ownership / usage of connection_cache Done http://gerrit.cloudera.org:8080/#/c/2574/14/be/src/util/hdfs-util.cc File be/src/util/hdfs-util.cc: Line 81: FilesystemsMatch > can you add some unit tests for this method? Trying to add tests for this is tricky. I will do it and upload a patch set, but meanwhile I don't want to hold up this patch set. Line 93: return true; > make one line Done Line 94: else > no need for 'else' if every block returns Done Line 95: strncmp > return strncmp(....) (and below) Done Line 101: // By this point we know that both the paths have schemes. > That may be true, but comment that the next step is to compare authorities. Done Line 102: += 3 > need to comment what this magic constant is doing. Done Line 107: namenode_a_len > for once I think a ternary would be clearer here: Done http://gerrit.cloudera.org:8080/#/c/2574/14/fe/src/main/java/com/cloudera/impala/common/FileSystemUtil.java File fe/src/main/java/com/cloudera/impala/common/FileSystemUtil.java: Line 90: if (!isDistributedFileSystem(p1) || !isDistributedFileSystem(p2)) return false; > Doesn't this method assume that p1 and p2 are on the same filesystem? What' No, p1 and p2 can be on different filesystems, we just check if they are on HDFS because arePathsInSameEncryptionZone() is a HDFS specific function. I've added another comment to clarify that. Line 168: Only distributed file systems have different encryption zones. > I think this comment (or something like it) belongs in arePathsInSameEncypt Done Line 168: If the source and : // destination are on different file systems, we need to use FileUtil.copy() to copy : // between filesystems. > "are on different file systems, or in different encryption zones, files can Done Line 173: sameEncryptionZone : : sameFileSystem; > for formatting this, prefer line breaking after the '=' and putting the who Done Line 277: Returns true iff the filesystem is a DistributedFileSystem. > Comment is wrong Done Line 284: Returns true iff the filesystem is a DistributedFileSystem. > Comment is wrong. Done -- To view, visit http://gerrit.cloudera.org:8080/2574 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I94e15ad67752dce21c9b7c1dced6e114905a942d Gerrit-PatchSet: 14 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
