Bharath Vissapragada 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 Done http://gerrit.cloudera.org:8080/#/c/8235/9//COMMIT_MSG@50 PS9, Line 50: speed up > speedup I see conflicting results. As per webster it is "speedup", but as per Oxford it is "speed-up". Now I don't know who to trust, but I'll go with your suggestion :) https://en.oxforddictionaries.com/definition/speed-up https://www.merriam-webster.com/dictionary/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" Done 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. Done 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 Done http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@232 PS9, Line 232: the > remove Done http://gerrit.cloudera.org:8080/#/c/8235/9/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@248 PS9, Line 248: runnable > callable Done 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 be Done 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 Alex to the rescue, good point. Made the change. 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. M Done 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) Done 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" Done 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 Done 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..". Yep, forgot to change here. 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 - I checked the callers and all of them just readers and either iterate / sets it inside another thrift object (which is when I think a copy is made). - Looks like ImmutableList.copyOf() doesn't always make a copy (although subject to change), using it here. 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... oops, yea, updated. -- 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 21:44:35 +0000 Gerrit-HasComments: Yes
