Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8235 )
Change subject: IMPALA-5429: Multi threaded block metadata loading ...................................................................... Patch Set 9: (16 comments) http://gerrit.cloudera.org:8080/#/c/8235/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8235/9//COMMIT_MSG@42 PS9, Line 42: datastructure nit: data structure http://gerrit.cloudera.org:8080/#/c/8235/9//COMMIT_MSG@50 PS9, Line 50: speed up speedup http://gerrit.cloudera.org:8080/#/c/8235/9/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/8235/9/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@122 PS9, Line 122: Limits the "Maximum" http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@223 PS9, Line 223: public FileMetadataLoadStats(Path path) { hdfsPath = path; } nit: move the ctor below the class variable members. http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@228 PS9, Line 228: nit: extra space http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@232 PS9, Line 232: the remove http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@248 PS9, Line 248: runnable callable http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@266 PS9, Line 266: // Computes the file metadata from scratch (by calling resetAndLoadFileMetadata()) : // when reuseFileMd_ is false, else refreshes the existing file metadata for : // modified files using refreshFileMetadata(). nit: you can actually remove the comment as it simply describes the code below. http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@465 PS9, Line 465: hasFileChanged I think you need to put back the caching check. Alex made a good point that it is used for updating the cached bytes size. http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@773 PS9, Line 773: loadParitionFileMetadataFromScratch "load from scratch" and "reset and load" essentially mean the same thing. Maybe rename this to resetAndLoadPartitionFileMetadata()? http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@792 PS9, Line 792: S3 non-HDFS (S3/ADLS) http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@792 PS9, Line 792: S3 "the latter" http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@844 PS9, Line 844: if (LOG.isTraceEnabled()) { : LOG.trace(loadStats.debugString()); : } nit: single line http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/service/BackendConfig.java File fe/src/main/java/org/apache/impala/service/BackendConfig.java: http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/service/BackendConfig.java@71 PS9, Line 71: maxS3PartsParallelLoad Why "maxS3..."? Everywhere else we use "maxNonHdfs..". http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/util/ListMap.java File fe/src/main/java/org/apache/impala/util/ListMap.java: http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/util/ListMap.java@43 PS9, Line 43: getList() { return list_; } Hm, why is list_ exposed to the world? You may want to check who wants this and what do they do with it (i.e modify or simply iterate). If it's the latter you can pass an ImmutableList(). If it is the former, see if we need to expand the public api of this class instead. http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/util/ListMap.java@76 PS9, Line 76: synchronized (this) { this is equivalent to public synchronized void populate... -- To view, visit http://gerrit.cloudera.org:8080/8235 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I07eaa7151dfc4d56da8db8c2654bd65d8f808481 Gerrit-Change-Number: 8235 Gerrit-PatchSet: 9 Gerrit-Owner: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Wed, 25 Oct 2017 19:24:35 +0000 Gerrit-HasComments: Yes
