Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12950 )
Change subject: Refactor file descriptor loading code ...................................................................... Patch Set 4: (6 comments) Looks pretty good to me, a bunch of nits and I can +2. http://gerrit.cloudera.org:8080/#/c/12950/4/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/4/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java@51 PS4, Line 51: private final Path partDir_; : private final ImmutableMap<String, FileDescriptor> oldFdsByName_; : private final ListMap<TNetworkAddress> hostIndex_; : : private boolean forceRefreshLocations = false; : : private List<FileDescriptor> loadedFds_; : private LoadStats loadStats_; javadocs http://gerrit.cloudera.org:8080/#/c/12950/4/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java@143 PS4, Line 143: Preconditions.checkNotNull(fd); : loadedFds_.add(fd); nit:merge? http://gerrit.cloudera.org:8080/#/c/12950/4/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java@162 PS4, Line 162: f (fileStatus instanceof LocatedFileStatus) { : locations = ((LocatedFileStatus)fileStatus).getBlockLocations(); : } else { : locations = fs.getFileBlockLocations(fileStatus, 0, fileStatus.getLen()); : } nit: document this behavior in the method doc? I feel the behavior is subtle since one of the branches is an RPC and the other is not. http://gerrit.cloudera.org:8080/#/c/12950/4/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java@171 PS4, Line 171: public List<FileDescriptor> getLoadedFds() { : return loadedFds_; : } : : public LoadStats getStats() { : return loadStats_; : } : : Path getPartDir() { : return partDir_; : } nit: single liners 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_); > I think that would increase coupling since, in order to init the FileMetada Fair point. Makes sense to me. http://gerrit.cloudera.org:8080/#/c/12950/4/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/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@545 PS4, Line 545: * @param isRefresh whether this is a refresh operation or an initial load. This only I think this is confusing. How about getting rid of this flag here and just say Loading block metadata for table foo, paths xxx and then in the actual FileMetadataLoader::cal() log whether it is refresh/load from scratch for each path? -- 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: 4 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: Sat, 13 Apr 2019 02:36:28 +0000 Gerrit-HasComments: Yes
