Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/20271 )
Change subject: IMPALA-12298: Improve incremental load of Iceberg tables ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/20271/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java File fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/20271/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@61 PS3, Line 61: private int newFilesThreshold_; This variable is not meant to change so I guess can be final as well. I see that some tests change the value of this but still this shouldn't be non-final for testing reasons in my opinion. What if we provided this value as a constructor param and the tests would exercise different threshold values via different instances of this class? http://gerrit.cloudera.org:8080/#/c/20271/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@77 PS3, Line 77: public void setNewFilesThreshold(int threshold) { This is only used for testing, right? Would it be possible to find a way not to expose this as public? @VisibleForTesting annotation maybe? Or if the threshold value is passed by a constructor param then this function won't be needed. http://gerrit.cloudera.org:8080/#/c/20271/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@99 PS3, Line 99: String msg = String.format("Refreshing Iceberg file metadata from path %s " + nit: I think this string could go directly into the LOG.trace() call without putting it onto a variable. http://gerrit.cloudera.org:8080/#/c/20271/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@215 PS3, Line 215: public boolean shouldReuseOldFds() throws IOException { Similarly to the setter above could this be private instead while somehow still visible for the tests? http://gerrit.cloudera.org:8080/#/c/20271/3/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/20271/3/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@983 PS3, Line 983: return relUri.getPath(); merge with line above http://gerrit.cloudera.org:8080/#/c/20271/3/fe/src/test/java/org/apache/impala/catalog/FileMetadataLoaderTest.java File fe/src/test/java/org/apache/impala/catalog/FileMetadataLoaderTest.java: http://gerrit.cloudera.org:8080/#/c/20271/3/fe/src/test/java/org/apache/impala/catalog/FileMetadataLoaderTest.java@279 PS3, Line 279: filesCanDataBeOutsideOfTableLocation nit: this name seems odd -- To view, visit http://gerrit.cloudera.org:8080/20271 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf643798a93e74ae7b0f37ceeab0a8052fb2699d Gerrit-Change-Number: 20271 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Mon, 31 Jul 2023 14:14:46 +0000 Gerrit-HasComments: Yes
