Henry Robinson 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? Line 95: HdfsOperationSet(HdfsFsCache::HdfsFsMap* connection_cache); Re-instate some comment about ownership / usage of connection_cache 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? Line 93: return true; make one line Line 94: else no need for 'else' if every block returns Line 95: strncmp return strncmp(....) (and below) 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. Line 102: += 3 need to comment what this magic constant is doing. Line 107: namenode_a_len for once I think a ternary would be clearer here: int namnode_a_len = (after_authority_a == NULL) ? strlen(path_a) : (after_authority_a - path_a) 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's the purpose of checking both paths here? Line 168: Only distributed file systems have different encryption zones. I think this comment (or something like it) belongs in arePathsInSameEncyptionZone() 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't be moved from one location to the other and must be copied instead." Line 173: sameEncryptionZone : : sameFileSystem; for formatting this, prefer line breaking after the '=' and putting the whole condition on one line where possible. Line 277: Returns true iff the filesystem is a DistributedFileSystem. Comment is wrong Line 284: Returns true iff the filesystem is a DistributedFileSystem. Comment is wrong. -- 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
