Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12950 )
Change subject: WIP: Refactor file descriptor loading code ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/12950/2/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java File fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/12950/2/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java@48 PS2, Line 48: Parallel > remove? Done http://gerrit.cloudera.org:8080/#/c/12950/2/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java@49 PS2, Line 49: private static final Configuration CONF = new Configuration(); > nit: Use the cached FileSystemUtil.CONF? since this is static anyway and Configuration objects are not a memory hog, I'd rather avoid a cross-class reference where unnecessary. (for many years now 'new Configuration()' has been pretty fast since it just keeps a pointer back to a common defaults reference rather than reloading the config files or anything like that) http://gerrit.cloudera.org:8080/#/c/12950/2/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java@58 PS2, Line 58: stats_ > nit: maybe call loadStats_ or something? There are too many stats in the co Done. Renamed the type to LoadStats as well. http://gerrit.cloudera.org:8080/#/c/12950/2/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/12950/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@560 PS2, Line 560: Map<Path, FileMetadataLoader> loadersByPath = Maps.newHashMap(); : for (Map.Entry<Path, List<HdfsPartition>> e : partsByPath.entrySet()) { : List<FileDescriptor> oldFds = e.getValue().get(0).getFileDescriptors(); : FileMetadataLoader loader = new FileMetadataLoader(e.getKey(), oldFds, hostIndex_); > does it make sense to move the loaders init into ParallelFileMetadata c'tor I think that would increase coupling since, in order to init the FileMetadataLoader, we need to look at some partition metadata info (L567 below), and I don't want to put HdfsPartition dependencies into the metadata-loading classes themselves. Though, maybe I misunderstood your suggestion. http://gerrit.cloudera.org:8080/#/c/12950/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@588 PS2, Line 588: // TODO(todd): it seems like the old implementation would actually modify I filed a bug here: https://issues.apache.org/jira/browse/IMPALA-8406 Probably merits some discussion on that JIRA what our intended behavior is supposed to be. The current behavior (prior to this patch) is not very good. The behavior on this patch might be incrementally better, so I'm tempted to leave as is and separately fix IMPALA-8406 in one of the two ways suggested there. http://gerrit.cloudera.org:8080/#/c/12950/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1083 PS2, Line 1083: * objects grouped by their base directory path. > Nit: Update javadoc Done -- To view, visit http://gerrit.cloudera.org:8080/12950 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I59edf493b9ba38be5f556b4795a7684d9c9e3a07 Gerrit-Change-Number: 12950 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Anonymous Coward (486) Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com> Gerrit-Comment-Date: Thu, 11 Apr 2019 00:07:08 +0000 Gerrit-HasComments: Yes
