Michael Smith 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 her sameEncryptionZone makes that redundant, but I guess it wouldn't hurt to keep around. 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 u I need to remove toString, that's implied by the conversion and in fact skipped later in this same function. My understanding of the conditional pattern is that it's to avoid string formatting when you're not going to use the results. However https://www.slf4j.org/faq.html#logging_performance discusses that; if you use slf4j's string substitution and avoid method calls, there's no overhead to calling LOG.trace directly vs guarding it with a conditional if you're not logging at that 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 a Ack -- 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 <michael.sm...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Comment-Date: Tue, 06 Dec 2022 21:42:48 +0000 Gerrit-HasComments: Yes
