Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/19262 )
Change subject: IMPALA-11736: Copy data between ofs buckets ...................................................................... Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/19262/5/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java: http://gerrit.cloudera.org:8080/#/c/19262/5/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@310 PS5, Line 310: boolean doRename = destIsDfs && sameEncryptionZone; I think we want to keep an equivalent to the "sameFileSystem" condition here (sameBucket would be fine). http://gerrit.cloudera.org:8080/#/c/19262/5/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@315 PS5, Line 315: LOG.trace("Moving '{}' to '{}'", sourceFile.toString(), destFile.toString()); Just to check my understanding: Are the "LOG.isTraceEnabled()" statements unnecessary because they are already controlled by the log level? http://gerrit.cloudera.org:8080/#/c/19262/5/fe/src/test/java/org/apache/impala/common/FileSystemUtilTest.java File fe/src/test/java/org/apache/impala/common/FileSystemUtilTest.java: http://gerrit.cloudera.org:8080/#/c/19262/5/fe/src/test/java/org/apache/impala/common/FileSystemUtilTest.java@20 PS5, Line 20: import org.apache.commons.lang3.tuple.Pair; Nit: Could you switch this to use org.apache.impala.common.Pair? (Just to avoid an extra external dependency) -- To view, visit http://gerrit.cloudera.org:8080/19262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic61f01672fa605fec0377885b13a1621573e424e Gerrit-Change-Number: 19262 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Tue, 06 Dec 2022 18:59:32 +0000 Gerrit-HasComments: Yes
