Noemi Pap-Takacs has posted comments on this change. ( http://gerrit.cloudera.org:8080/22014 )
Change subject: IMPALA-13154: Update metrics when loading an HDFS table ...................................................................... Patch Set 22: (3 comments) Looks nice, thanks! http://gerrit.cloudera.org:8080/#/c/22014/22/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/22014/22/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@390 PS22, Line 390: add(FileDescriptor fd) nit: this name and signature is a bit misleading, since we do not actually add the FD itself, just recalculate the stats. http://gerrit.cloudera.org:8080/#/c/22014/22/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/22014/22/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@151 PS22, Line 151: fileMetadataStats_.add(fd); : ++loadStats_.loadedFiles; My suggestion earlier was to merge LoadStats and FileMetadataStats since they are both collected at the same time in (Iceberg)FileMetadataLoader and have some common fields. LoadStats have more details about blocks, storage ids, etc, which are only used for testing AFAIK but I don't know how relevant these stats are now. I am fine with it as it is now, maybe it can be refactored later. What are your thoughts on this? http://gerrit.cloudera.org:8080/#/c/22014/22/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@201 PS22, Line 201: Preconditions.checkNotNull(fd) nit: fd can't be null in this branch -- To view, visit http://gerrit.cloudera.org:8080/22014 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6e2eb503b0f61b1e6403058bc5dc78d721e7e940 Gerrit-Change-Number: 22014 Gerrit-PatchSet: 22 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 20 Dec 2024 10:55:51 +0000 Gerrit-HasComments: Yes
