Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22872 )
Change subject: IMPALA-12162: Use thread pool to collect checksums ...................................................................... Patch Set 12: (3 comments) http://gerrit.cloudera.org:8080/#/c/22872/12/fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java File fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/22872/12/fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java@126 PS12, Line 126: TODO: does this need to be sequential? I don't think we need. partBuilders are independent to each other. http://gerrit.cloudera.org:8080/#/c/22872/12/fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java@128 PS12, Line 128: for (HdfsPartition.Builder partBuilder : builders) { : // Checks if we can reuse the old file descriptors. Partition builders in the : // list may have different file descriptors. We need to verify them one by one. : if ((!loader.hasFilesChangedCompareTo(partBuilder.getFileDescriptors()))) { : LOG.trace("Detected files unchanged on partition {}", : partBuilder.getPartitionName()); : continue; : } : partBuilder.clearFileDescriptors(); : List<FileDescriptor> deleteDescriptors = loader.getLoadedDeleteDeltaFds(); : if (deleteDescriptors != null && !deleteDescriptors.isEmpty()) { : partBuilder.setInsertFileDescriptors(loader.getLoadedInsertDeltaFds()); : partBuilder.setDeleteFileDescriptors(loader.getLoadedDeleteDeltaFds()); : } else { : partBuilder.setFileDescriptors(loader.getLoadedFds()); : } : partBuilder.setFileMetadataStats(loader.getFileMetadataStats()); : } : }); nit: it'd be more clean if we wrap these into a method, e.g. private static void updatePartBuilders(FileMetadataLoader loader, List<HdfsPartition.Builder> builders) { http://gerrit.cloudera.org:8080/#/c/22872/12/fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java@193 PS12, Line 193: Thread.currentThread().interrupt(); This seems something new added by this patch. Any reason for it? -- To view, visit http://gerrit.cloudera.org:8080/22872 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I314621104e4757620c0a90d41dd6875bf8855b51 Gerrit-Change-Number: 22872 Gerrit-PatchSet: 12 Gerrit-Owner: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Comment-Date: Thu, 22 May 2025 01:55:54 +0000 Gerrit-HasComments: Yes
