Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13806 )
Change subject: IMPALA-6663 Expose current DDL metrics on WebUI ...................................................................... Patch Set 7: (4 comments) I think this in general is a very useful addition. Can you also specify the motivation of why we need to track the total number of DDLs instead of tracking them on a per table level. Certain metadata operations like invalidate metadata as indeed global and cannot be tracked at table. However, I think it useful to track how many refresh or invalidates were called on a given table and finding top-n such tables. Similarly for DMLs, instead of a getting a total count of INSERTS, would it be more useful to get top-N tables which are inserted into? http://gerrit.cloudera.org:8080/#/c/13806/7/fe/src/main/java/org/apache/impala/catalog/monitor/CatalogOpRequestCounter.java File fe/src/main/java/org/apache/impala/catalog/monitor/CatalogOpRequestCounter.java: http://gerrit.cloudera.org:8080/#/c/13806/7/fe/src/main/java/org/apache/impala/catalog/monitor/CatalogOpRequestCounter.java@41 PS7, Line 41: private AtomicLong resetMetadataCounter_; : : private AtomicLong dmlOperationCounter_; : : private AtomicLong syncDdlOperationCounter_; Can you add a one line comment on each of these counters as to what do they represent? Also, would be great if you could specify what operations constitute a DML in this context. http://gerrit.cloudera.org:8080/#/c/13806/7/fe/src/main/java/org/apache/impala/catalog/monitor/CatalogOpRequestCounter.java@70 PS7, Line 70: decrementDdlTypeCounter Is decrement ever needed? same for the other counters? http://gerrit.cloudera.org:8080/#/c/13806/7/fe/src/main/java/org/apache/impala/catalog/monitor/CatalogOpRequestCounter.java@109 PS7, Line 109: getResetMetadataCounter By returning the AtomicLong object this class is leaking the private field and it is subject to modification by other classes. A more common pattern is to return its value (return type long). http://gerrit.cloudera.org:8080/#/c/13806/7/www/catalog.tmpl File www/catalog.tmpl: http://gerrit.cloudera.org:8080/#/c/13806/7/www/catalog.tmpl@202 PS7, Line 202: Running Is this user-visible text? Running seems to suggest these operations are currently being run. -- To view, visit http://gerrit.cloudera.org:8080/13806 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0ed76f134bad6d3b3d4dce132365a53a01e9512a Gerrit-Change-Number: 13806 Gerrit-PatchSet: 7 Gerrit-Owner: Tamas Mate <tm...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Tue, 13 Aug 2019 17:24:38 +0000 Gerrit-HasComments: Yes
