Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8529 )
Change subject: [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI. ...................................................................... Patch Set 3: Code-Review+1 (22 comments) Keeping +1 from Vuk http://gerrit.cloudera.org:8080/#/c/8529/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8529/3//COMMIT_MSG@7 PS3, Line 7: [PREVIEW] > Remove? Done http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.cc@492 PS2, Line 492: // TODO: Enable json view of table metrics > I think thats a one-liner looking at other places in the code. The URL for table metrics already has a "?", so I am not sure how the ?json will be added. It works fine for other places like /catalog. If I do what you suggested it gives me a raw html :) http://gerrit.cloudera.org:8080/#/c/8529/3/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/8529/3/be/src/catalog/catalog-server.cc@359 PS3, Line 359: GetCatalogUsage(document); > Check the return value and return early if it threw a failure? Why abandon loading the rest of the /catalog page if the catalog usage call fails? The rest of the stuff loaded in this function don't depend on the catalog usage information. If the former fails, an error will be printed in the corresponding section. http://gerrit.cloudera.org:8080/#/c/8529/3/be/src/catalog/catalog-server.cc@516 PS3, Line 516: object_name > name? Done http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java File fe/src/main/java/org/apache/impala/catalog/Catalog.java: http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java@166 PS3, Line 166: if (tbl != null) CatalogUsageMonitor.INSTANCE.removeTable(tbl); > I think this is only executed on the Catalog service side and it looks to m Done http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1395 PS3, Line 1395: metris > nit: metrics Done http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1395 PS3, Line 1395: pretty-prints them into a : * string. Returns the string representation of the table metrics > condense: ... and returns a pretty-printed string representation. Done http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@934 PS3, Line 934: thriftHdfsPart.setHas_incremental_stats(hasIncrementalStats()); > IMO, we should also add the incremental stats size estimate by computing th That happens already in HdfsTable.java:L1737. http://gerrit.cloudera.org:8080/#/c/8529/3/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/8529/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@196 PS3, Line 196: > ..any of the partitions in.. ? Done http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@252 PS3, Line 252: functionality > nit: purposes Done http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1720 PS3, Line 1720: metadataSizeEstimate > may be memUsageEstimate to be inline with CatalogUsage..terminology? Done http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1743 PS3, Line 1743: stats.numBlocks += tHdfsPartition.getNum_blocks(); : stats.numFiles += : tHdfsPartition.isSetFile_desc() ? tHdfsPartition.getFile_desc().size() : 0; : stats.totalFileBytes += tHdfsPartition.getTotal_file_size_bytes(); > Shouldn't these be populated irrespective of "includeFileDesc" since they a includeFileDesc is always true when this is called in the catalogd. For the impalad case, we don't really care. Also, when includeFileDesc is false, we don't iterate over files and blocks so, it would result in unnecessary overhead to always do that. http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/Table.java@151 PS3, Line 151: public Metrics getMetrics() { return metrics_; } > Preconditions.checkState(tableLock_.isHeldByCurrentThread())? Hm, not sure if we need to add this check to every public function of table. That seem a bit too much. What do you think? http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/Table.java@579 PS3, Line 579: trully > nit: truly Done http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@362 PS3, Line 362: final Timer.Context context > Include the lock in the context? That may not be a good idea since we will be accessing a table field in an unprotected fashion. http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3334 PS3, Line 3334: */ > Maybe add a comment that this increments the opsCount, otherwise, some call Done http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/util/TopNCache.java File fe/src/main/java/org/apache/impala/util/TopNCache.java: http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/util/TopNCache.java@56 PS3, Line 56: function_ = f; > Preconditions.checkState(maxCapacity > 0)? Done http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/util/TopNCache.java@62 PS3, Line 62: function_.apply(t1).compareTo(function_.apply(t2)) > nit: factor the two calls with a method called compareRanks(t1, t2) ? Done http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/util/TopNCache.java@78 PS3, Line 78: if (!heap_.remove(item)) > easier to read? That wouldn't be correct. Keep in mind this is a put or update function. We still need to overwrite the element in the cache with 'item'. http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/test/java/org/apache/impala/util/TestTopNCache.java File fe/src/test/java/org/apache/impala/util/TestTopNCache.java: http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/test/java/org/apache/impala/util/TestTopNCache.java@39 PS3, Line 39: TopNCache<Long, Long> cache = : new TopNCache<Long, Long>(new Function<Long, Long>() { : @Override : public Long apply(Long element) { return element; } : }, capacity, policy); : // populate with more values that the specified max capacity : for (long i = 0; i < capacity * 2; i++) cache.putOrUpdate(i); : assertEquals(cache.listEntries().size(), capacity); > I think the TopNCache creation with a specified evictionpolicy and insert t Done http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/test/java/org/apache/impala/util/TestTopNCache.java@52 PS3, Line 52: > Also assert the ranking by randomizing the puts? Done http://gerrit.cloudera.org:8080/#/c/8529/3/www/catalog.tmpl File www/catalog.tmpl: http://gerrit.cloudera.org:8080/#/c/8529/3/www/catalog.tmpl@135 PS3, Line 135: <td><a href="table_metrics?name={{fqtn}}">{{name}}-metrics</a></td> > add some unit tests for this? (test_web_pages.py) Done -- To view, visit http://gerrit.cloudera.org:8080/8529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I37d407979e6d3b1a444b6b6265900b148facde9e Gerrit-Change-Number: 8529 Gerrit-PatchSet: 3 Gerrit-Owner: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Sat, 16 Dec 2017 02:07:15 +0000 Gerrit-HasComments: Yes
