You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org> on 2017/11/13 19:30:00 UTC
[Impala-ASF-CR] [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
Dimitris Tsirogiannis has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8529
Change subject: [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
......................................................................
[PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
The following changes are included in this commit:
* Adds a lightweight framework for registering metrics in the JVM.
* Adds table-level metrics and enables these metrics to be exposed
through the catalog web UI.
* Adds a CatalogUsageMonitor class that monitors and reports the catalog
usage in terms of the tables with the highest memory requirements and
the tables with the highest number of metadata operations. The catalog
usage information is exposed in the /catalog page of the catalog web UI.
Change-Id: I37d407979e6d3b1a444b6b6265900b148facde9e
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M common/thrift/Frontend.thrift
M common/thrift/JniCatalog.thrift
M fe/pom.xml
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java
M fe/src/main/java/org/apache/impala/catalog/HBaseTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
A fe/src/main/java/org/apache/impala/common/Metrics.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/util/TopNCache.java
M www/catalog.tmpl
A www/table_metrics.tmpl
21 files changed, 953 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/8529/1
--
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: newchange
Gerrit-Change-Id: I37d407979e6d3b1a444b6b6265900b148facde9e
Gerrit-Change-Number: 8529
Gerrit-PatchSet: 1
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
[Impala-ASF-CR] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8529 )
Change subject: IMPALA-4886: Expose table metrics in the catalog web UI.
......................................................................
Patch Set 5: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1755/
--
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: 5
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Jan 2018 03:03:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8529 )
Change subject: IMPALA-4886: Expose table metrics in the catalog web UI.
......................................................................
Patch Set 5: Code-Review+2
Rebase
--
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: 5
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Jan 2018 23:26:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
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 1:
Screenshots of the catalog web UI to illustrate the changes:
https://cloudera.box.com/s/tgvt0yc3mfhvc25i2tm30m3h49769jfp
https://cloudera.box.com/s/hc0ru8q2ajfne6gzy5x4s5ri5p6kgawz
--
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: 1
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Comment-Date: Mon, 13 Nov 2017 19:40:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Philip Zeyliger, Vuk Ercegovac,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8529
to look at the new patch set (#5).
Change subject: IMPALA-4886: Expose table metrics in the catalog web UI.
......................................................................
IMPALA-4886: Expose table metrics in the catalog web UI.
The following changes are included in this commit:
* Adds a lightweight framework for registering metrics in the JVM.
* Adds table-level metrics and enables these metrics to be exposed
through the catalog web UI.
* Adds a CatalogUsageMonitor class that monitors and reports the catalog
usage in terms of the tables with the highest memory requirements and
the tables with the highest number of metadata operations. The catalog
usage information is exposed in the /catalog page of the catalog web UI.
Change-Id: I37d407979e6d3b1a444b6b6265900b148facde9e
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M common/thrift/CatalogObjects.thrift
M common/thrift/Frontend.thrift
M common/thrift/JniCatalog.thrift
M fe/pom.xml
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java
M fe/src/main/java/org/apache/impala/catalog/HBaseTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
A fe/src/main/java/org/apache/impala/common/Metrics.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/util/TopNCache.java
A fe/src/test/java/org/apache/impala/util/TestTopNCache.java
M tests/webserver/test_web_pages.py
M www/catalog.tmpl
A www/table_metrics.tmpl
24 files changed, 1,206 insertions(+), 113 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/8529/5
--
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: newpatchset
Gerrit-Change-Id: I37d407979e6d3b1a444b6b6265900b148facde9e
Gerrit-Change-Number: 8529
Gerrit-PatchSet: 5
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada 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:
(20 comments)
The patch mostly lgtm. I have some minor suggestions before I can +1.
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?
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@359
PS2, Line 359: GetCatalogUsage(document);
> In principal, that's a good idea. I plan on doing it in the future for more
Makes sense. If you don't mind, can you please raise jiras for these to-dos so that we don't lose track of them. Also, they sound like great "ramp-up" tasks, that others can pick up.
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
> Good idea. Left a TODO.
I think thats a one-liner looking at other places in the code.
document->AddMember(Webserver::ENABLE_RAW_JSON_KEY, true, document->GetAllocator());
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?
if (!GetCatalogUsage().ok()) return;
http://gerrit.cloudera.org:8080/#/c/8529/3/be/src/catalog/catalog-server.cc@516
PS3, Line 516: object_name
name?
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog.h
File be/src/catalog/catalog.h:
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog.h@96
PS2, Line 96: Status GetCatalogUsage(TGetCatalogUsageResponse* response);
> I renamed the other one to GetCatalogUsage. I thought of keeping it a bit m
Agree.
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 me like the CatalogUsageMonitor is populated on the coordinators too. May be we should just restrict it to the catalogd?
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java
File fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java:
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java@47
PS2, Line 47: true
> I wanted to avoid the scenario where 25 tables that were accessed in the pa
Oh, I see your point. It can be argued both ways.
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 the obj sizes of the params map (if incr stats are present of course).
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.. ?
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?
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 account for memory usage?
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())?
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/common/Metrics.java
File fe/src/main/java/org/apache/impala/common/Metrics.java:
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/common/Metrics.java@143
PS2, Line 143:
> Not sure I follow. Why le? I just want to print the % to indicate percentil
I thought % is used for percentage and %le or %tile for percentile to differentiate. I'm not too strong about this.
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?
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 callers might call it repeatedly resulting in spurious counts.
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)?
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 types (random/sequential) can be factored out into a helper.
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?
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)
--
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 <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Dec 2017 22:34:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac 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
(6 comments)
mostly small comments and optional suggestions.
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
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.
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@252
PS3, Line 252: functionality
nit: purposes
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@579
PS3, Line 579: trully
nit: truly
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@62
PS3, Line 62: function_.apply(t1).compareTo(function_.apply(t2))
nit: factor the two calls with a method called compareRanks(t1, t2) ?
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?
if (heap_.contains(item)) return;
--
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 <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2017 02:42:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8529 )
Change subject: IMPALA-4886: Expose table metrics in the catalog web UI.
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1754/
--
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: 5
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Jan 2018 23:26:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac 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 1:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/8529/1/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
http://gerrit.cloudera.org:8080/#/c/8529/1/be/src/catalog/catalog-server.cc@391
PS1, Line 391: DCHECK_EQ(catalog_usage_result.large_tables.size(),
: catalog_usage_result.memory_estimates.size());
: DCHECK_EQ(catalog_usage_result.frequent_tables.size(),
: catalog_usage_result.num_metadata_operations.size());
can be removed if using a struct instead of multiple arrays
http://gerrit.cloudera.org:8080/#/c/8529/1/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/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1091
PS1, Line 1091: REFRESH
why not sync this name to RELOAD?
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java
File fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java:
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java@27
PS1, Line 27: Sigleton
nit(spelling): Singleton
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java@48
PS1, Line 48: true
why always evict for this one?
http://gerrit.cloudera.org:8080/#/c/8529/1/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/1/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@923
PS1, Line 923: HdfsTable.StorageStats stats,
: Reference<Boolean> hasIncrementalStats
this is a surprising api since it modifies the last two args and does a bit more than "toThrift". I'd keep the two parts (serializing to thrift and collecting stats) separate.
http://gerrit.cloudera.org:8080/#/c/8529/1/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/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@197
PS1, Line 197: al stats
this seems to only be written, not read. can it be removed or will it be used for something? if its kept, then clarify what it means-- currently it looks like its flipped when some partition is incremental.
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@220
PS1, Line 220: table or partition
: // level.
unclear what this is trying to say. is this: "aggregated table wide at the granularity of a partition"?
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@256
PS1, Line 256: from
nit: by
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1735
PS1, Line 1735: hasIncrementalStats.getRef()
there's a lot going on here-- surprising to see that its tested given that its declared a few lines back. see the comment on toThrift in hdfsPartition. why not just get this bool from the partition?
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1737
PS1, Line 1737: hasIncrementalStats_
so incremental stats is a table-wide property and stored per partition? so some partitions can have incremental stats and others not?
http://gerrit.cloudera.org:8080/#/c/8529/1/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/1/fe/src/main/java/org/apache/impala/util/TopNCache.java@35
PS1, Line 35: Always evict policy
why is this needed? is it to reflect some sort of recency?
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/util/TopNCache.java@94
PS1, Line 94: .
do you want to enforce a sort order here or at the caller? might be simpler here since you know the comparison function.
http://gerrit.cloudera.org:8080/#/c/8529/1/www/catalog.tmpl
File www/catalog.tmpl:
http://gerrit.cloudera.org:8080/#/c/8529/1/www/catalog.tmpl@26
PS1, Line 26: Top-25
if parameterizing N, this would need to change as well. perhaps omit the N?
http://gerrit.cloudera.org:8080/#/c/8529/1/www/catalog.tmpl@34
PS1, Line 34:
several ws issues here.
http://gerrit.cloudera.org:8080/#/c/8529/1/www/catalog.tmpl@65
PS1, Line 65: Top-25
same here
http://gerrit.cloudera.org:8080/#/c/8529/1/www/catalog.tmpl@78
PS1, Line 78: {{#frequent_tables}}
are these sorted (desc) by num operations? the screenshot for this one is not sorted.
--
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: 1
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Nov 2017 19:37:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Philip Zeyliger, Vuk Ercegovac,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8529
to look at the new patch set (#3).
Change subject: [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
......................................................................
[PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
The following changes are included in this commit:
* Adds a lightweight framework for registering metrics in the JVM.
* Adds table-level metrics and enables these metrics to be exposed
through the catalog web UI.
* Adds a CatalogUsageMonitor class that monitors and reports the catalog
usage in terms of the tables with the highest memory requirements and
the tables with the highest number of metadata operations. The catalog
usage information is exposed in the /catalog page of the catalog web UI.
Change-Id: I37d407979e6d3b1a444b6b6265900b148facde9e
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M common/thrift/CatalogObjects.thrift
M common/thrift/Frontend.thrift
M common/thrift/JniCatalog.thrift
M fe/pom.xml
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java
M fe/src/main/java/org/apache/impala/catalog/HBaseTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
A fe/src/main/java/org/apache/impala/common/Metrics.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/util/TopNCache.java
A fe/src/test/java/org/apache/impala/util/TestTopNCache.java
M www/catalog.tmpl
A www/table_metrics.tmpl
23 files changed, 1,179 insertions(+), 133 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/8529/3
--
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: newpatchset
Gerrit-Change-Id: I37d407979e6d3b1a444b6b6265900b148facde9e
Gerrit-Change-Number: 8529
Gerrit-PatchSet: 3
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
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 2:
(28 comments)
Thanks for the review Bharath. Many nice suggestions. There will be a sequence of patches to improve our ability to monitor/debug the catalog. I just want to avoid cramming too much stuff in a single patch which is already kind of big.
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.h
File be/src/catalog/catalog-server.h:
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.h@201
PS2, Line 201: used
> Clarify what is 'used'?
Done
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.h@217
PS2, Line 217: Update
> nit: May be 'GetTopNTableUsageMetrics()' or something (I know it sounds hor
Done
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.h@224
PS2, Line 224: object_name
> nit: probably just 'name'? Its implicit (from the URL) that its a table.
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@359
PS2, Line 359: UpdateCatalogUsage(document);
> Whats your take on moving top-x stuff to the /metrics page and populating t
In principal, that's a good idea. I plan on doing it in the future for more generic catalog metrics. It will require some thinking into how to best integrate the JVM metrics with the metrics management system we use in the backend, so I thought of postponing it for another patch.
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.cc@390
PS2, Line 390: has_metrics.SetBool(true);
> Just curious, whats the use of has_metrics=true/has_large_tables=true? We c
That's for preventing the impalad web ui from trying to print the catalog usage metrics. I had issue doing this from mustache, so I resorted to using this boolean flag.
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.cc@492
PS2, Line 492: Webserver::ArgumentMap::const_iterator object_name_arg = args.find("object_name");
> should we enable json view for this? (?json)
Good idea. Left a TODO.
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog.h
File be/src/catalog/catalog.h:
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog.h@90
PS2, Line 90: in
> nit: into?
Done
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog.h@96
PS2, Line 96: Status GetCatalogUsage(TGetCatalogUsageResponse* response);
> Same comment as in the other header, may be rename to convey that it is Get
I renamed the other one to GetCatalogUsage. I thought of keeping it a bit more generic in case we want to add other stuff as well. Thoughts?
http://gerrit.cloudera.org:8080/#/c/8529/2/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/8529/2/common/thrift/Frontend.thrift@90
PS2, Line 90: // Arguments to getTableMetrics, which returns the metrics of a specific table.
: struct TGetTableMetricsParams {
: 1: required string db
: 2: required string tbl
: }
:
: // Response to a getTableMetrics request. The response contains all the collected metrics
: // pretty-printed into a string.
: struct TGetTableMetricsResponse {
: 1: required string metrics
: }
> Can't we use TTableName and String directly? (or) probably use typedefs
Good point about TTableName (used that). I kept the TGetTableMetrics* structs for consistency with the other API calls.
http://gerrit.cloudera.org:8080/#/c/8529/2/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
http://gerrit.cloudera.org:8080/#/c/8529/2/common/thrift/JniCatalog.thrift@604
PS2, Line 604: Currently, it keeps track of the
: // estimated memory usage and the number of metadata operations that were performed on
: // that table since it was loaded.
> Remove here and describe below may be, can likely become stale soon (Same f
Done
http://gerrit.cloudera.org:8080/#/c/8529/2/common/thrift/JniCatalog.thrift@607
PS2, Line 607: Stats
> Looks like we are using Stats/Metrics interchangeably. Maybe use Metrics ev
Good point. Done
http://gerrit.cloudera.org:8080/#/c/8529/2/common/thrift/JniCatalog.thrift@608
PS2, Line 608: required string table_name
> TTableName?
Done
http://gerrit.cloudera.org:8080/#/c/8529/2/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/2/fe/src/main/java/org/apache/impala/catalog/Catalog.java@154
PS2, Line 154: Table tbl = db.getTable(tableName);
: if (tbl != null) {
: tbl.incrementMetadataOpsCount();
: CatalogUsageMonitor.INSTANCE.updateFrequentlyAccessedTables(tbl);
: }
> Looks subtle and flaky to me. This seems to work on the premise that we cal
Done
http://gerrit.cloudera.org:8080/#/c/8529/2/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/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1007
PS2, Line 1007: final Timer.Context context
> Should the scope include tryLockTable()?
Done
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1372
PS2, Line 1372: public TGetCatalogUsageResponse getCatalogUsage() {
> getTopN...Metrics?
See previous comment. Thought best to keep it more generic. Let me know what you think.
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1391
PS2, Line 1391: public String getTableMetrics(String dbName, String tblName) throws CatalogException {
> Doc?
Done
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1392
PS2, Line 1392: Table tbl = getTable(dbName, tblName);
> This is the problem I was talking about. This call increments the metadatOp
Yeap, good point. Fixed it.
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1398
PS2, Line 1398: No metrics available for table " + dbName + "." + tblName
> Also may be add "Table not yet loaded..."? Its unclear why metrics are not
Done
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java
File fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java:
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java@47
PS2, Line 47: true
> It is unclear why evict should be true (or) whats the usecase for that.
I wanted to avoid the scenario where 25 tables that were accessed in the past prevent any other table from showing in the top n. In your example, if the 25 tables are still relevant (accessed frequently), they will keep showing up in the topN. Essentially, all the infrequent ones will be competing for the last spot in the list. Obviously, there are other better ways to implement something like that but that was just one simple approach. What I really want is an LFU cache with dynamic aging.
http://gerrit.cloudera.org:8080/#/c/8529/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/8529/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@221
PS2, Line 221: StorageStats
> Maybe call FileMetadataStats to be ni sync with rest of the class?
Done
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1201
PS2, Line 1201: public void load(boolean reuseMetadata, IMetaStoreClient client,
> Make rest of the load()'s private so that all loads happen via this call an
Unfortunately, they both need to be public (sigh). Since the former calls the latter, I moved the timer there to make sure we capture all calls.
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@234
PS2, Line 234: kuduTableName_, e);
> formatting (below too)
Done
http://gerrit.cloudera.org:8080/#/c/8529/2/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/2/fe/src/main/java/org/apache/impala/catalog/Table.java@135
PS2, Line 135: public void setEstimatedMetadataSize(long estimatedMetadataSize) {
: estimatedMetadataSize_.set(estimatedMetadataSize);
: }
:
: public void incrementMetadataOpsCount() { metadataOpsCount_.incrementAndGet(); }
> Can these make a call to CatalogUsageMonitor.increment*(this) too? We can a
Good point. Done
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/common/Metrics.java
File fe/src/main/java/org/apache/impala/common/Metrics.java:
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/common/Metrics.java@143
PS2, Line 143:
> %le ?
Not sure I follow. Why le? I just want to print the % to indicate percentile.
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/common/Metrics.java@147
PS2, Line 147: }
> Same as my comment in the backend, can we expose json versions of these? to
Added a TODO.
http://gerrit.cloudera.org:8080/#/c/8529/2/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/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@259
PS2, Line 259:
> Can we add gauge metrics around this switch-case block to count DDLs by typ
Good point. That will be added in a follow up patch where we will add these types of metrics (not tied to specific objects). We also want to add a a view of in-flight catalog operations (similar to what impalads have in queries/ tab).
http://gerrit.cloudera.org:8080/#/c/8529/2/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/2/fe/src/main/java/org/apache/impala/util/TopNCache.java@41
PS2, Line 41: R extends Long>
> Do we need this? It is almost always Long, no?
Yes, it's kind of annoying to have to specify it here. I need it for the function_ declaration though.
http://gerrit.cloudera.org:8080/#/c/8529/2/www/catalog.tmpl
File www/catalog.tmpl:
http://gerrit.cloudera.org:8080/#/c/8529/2/www/catalog.tmpl@23
PS2, Line 23: {{?has_large_tables}}
> Like I mentioned elsewhere, my feeling is that these belong to metrics and
You can get ?json of that page and get the metrics (I just tried it). We need to think about the catalog metrics a bit more. The metrics exposed in this patch are tied to specific objects (tables that come and go). To my mind there is another class of "generic" metrics to capture the catalog's behavior and I think this should definitely be in the /metrics page.
--
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: 2
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Dec 2017 19:43:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8529 )
Change subject: IMPALA-4886: Expose table metrics in the catalog web UI.
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1759/
--
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: 6
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Jan 2018 05:44:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada 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 2:
(28 comments)
Great change, exposes some invaluable information when debugging Catalog issues, which otherwise is a black box.
I still need to look deeper into some parts of the code around HdfsTable.toThrift() stats accounting and TopNCache unit tests. Flushing out what I have so far.
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.h
File be/src/catalog/catalog-server.h:
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.h@201
PS2, Line 201: used
Clarify what is 'used'?
Maybe we should also mention that its top-n and not everything (with a reference to the fe call)
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.h@217
PS2, Line 217: Update
nit: May be 'GetTopNTableUsageMetrics()' or something (I know it sounds horrible :D)? My thinking is that we are not fully accounting for the entire usage and the method name could be misleading.
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.h@224
PS2, Line 224: object_name
nit: probably just 'name'? Its implicit (from the URL) that its a table.
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@359
PS2, Line 359: UpdateCatalogUsage(document);
Whats your take on moving top-x stuff to the /metrics page and populating them like other metrics? I know some downstream clients (like CM) read that page and populate nice charts on how those metrics change over time.
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.cc@390
PS2, Line 390: has_metrics.SetBool(true);
Just curious, whats the use of has_metrics=true/has_large_tables=true? We could just read the json and see if they exist?
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.cc@492
PS2, Line 492: Webserver::ArgumentMap::const_iterator object_name_arg = args.find("object_name");
should we enable json view for this? (?json)
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog.h
File be/src/catalog/catalog.h:
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog.h@90
PS2, Line 90: in
nit: into?
http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog.h@96
PS2, Line 96: Status GetCatalogUsage(TGetCatalogUsageResponse* response);
Same comment as in the other header, may be rename to convey that it is GetTopN....()?
http://gerrit.cloudera.org:8080/#/c/8529/2/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/8529/2/common/thrift/Frontend.thrift@90
PS2, Line 90: // Arguments to getTableMetrics, which returns the metrics of a specific table.
: struct TGetTableMetricsParams {
: 1: required string db
: 2: required string tbl
: }
:
: // Response to a getTableMetrics request. The response contains all the collected metrics
: // pretty-printed into a string.
: struct TGetTableMetricsResponse {
: 1: required string metrics
: }
Can't we use TTableName and String directly? (or) probably use typedefs
typedef TTableName TGetTableMetricsParams
typedef string TGetTableMetricsResponse
http://gerrit.cloudera.org:8080/#/c/8529/2/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
http://gerrit.cloudera.org:8080/#/c/8529/2/common/thrift/JniCatalog.thrift@604
PS2, Line 604: Currently, it keeps track of the
: // estimated memory usage and the number of metadata operations that were performed on
: // that table since it was loaded.
Remove here and describe below may be, can likely become stale soon (Same for the next struct too)
http://gerrit.cloudera.org:8080/#/c/8529/2/common/thrift/JniCatalog.thrift@607
PS2, Line 607: Stats
Looks like we are using Stats/Metrics interchangeably. Maybe use Metrics everywhere since "Stats" can coincide with table stats.
http://gerrit.cloudera.org:8080/#/c/8529/2/common/thrift/JniCatalog.thrift@608
PS2, Line 608: required string table_name
TTableName?
http://gerrit.cloudera.org:8080/#/c/8529/2/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/2/fe/src/main/java/org/apache/impala/catalog/Catalog.java@154
PS2, Line 154: Table tbl = db.getTable(tableName);
: if (tbl != null) {
: tbl.incrementMetadataOpsCount();
: CatalogUsageMonitor.INSTANCE.updateFrequentlyAccessedTables(tbl);
: }
Looks subtle and flaky to me. This seems to work on the premise that we call Catalog.getTable() only for a metadata-op? Future patches could likely break the counts if they don't know this?
May be we could move this to CatalogOpEx.getExistingTable() (Its private and so is only called from inside for DDLs)
http://gerrit.cloudera.org:8080/#/c/8529/2/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/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1007
PS2, Line 1007: final Timer.Context context
Should the scope include tryLockTable()?
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1372
PS2, Line 1372: public TGetCatalogUsageResponse getCatalogUsage() {
getTopN...Metrics?
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1391
PS2, Line 1391: public String getTableMetrics(String dbName, String tblName) throws CatalogException {
Doc?
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1392
PS2, Line 1392: Table tbl = getTable(dbName, tblName);
This is the problem I was talking about. This call increments the metadatOpsCount() as per this patch.
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1398
PS2, Line 1398: No metrics available for table " + dbName + "." + tblName
Also may be add "Table not yet loaded..."? Its unclear why metrics are not available yet.
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java
File fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java:
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java@47
PS2, Line 47: true
It is unclear why evict should be true (or) whats the usecase for that.
Let's say we have 25 tables each accessed 100 times and a 26th table that is accessed for the first time, this makes sure that the last one of 25 is evicted, now we have 24 tables with 100 metadataops count and 1 table with 1 metadata op count. As a user I think I'd be more interested in the former? No?
http://gerrit.cloudera.org:8080/#/c/8529/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/8529/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@221
PS2, Line 221: StorageStats
Maybe call FileMetadataStats to be ni sync with rest of the class?
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1201
PS2, Line 1201: public void load(boolean reuseMetadata, IMetaStoreClient client,
Make rest of the load()'s private so that all loads happen via this call and accounted?
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@234
PS2, Line 234: kuduTableName_, e);
formatting (below too)
http://gerrit.cloudera.org:8080/#/c/8529/2/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/2/fe/src/main/java/org/apache/impala/catalog/Table.java@135
PS2, Line 135: public void setEstimatedMetadataSize(long estimatedMetadataSize) {
: estimatedMetadataSize_.set(estimatedMetadataSize);
: }
:
: public void incrementMetadataOpsCount() { metadataOpsCount_.incrementAndGet(); }
Can these make a call to CatalogUsageMonitor.increment*(this) too? We can avoid making a separate call at multiple places.
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/common/Metrics.java
File fe/src/main/java/org/apache/impala/common/Metrics.java:
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/common/Metrics.java@143
PS2, Line 143:
%le ?
http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/common/Metrics.java@147
PS2, Line 147: }
Same as my comment in the backend, can we expose json versions of these? toJson()
http://gerrit.cloudera.org:8080/#/c/8529/2/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/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@259
PS2, Line 259:
Can we add gauge metrics around this switch-case block to count DDLs by type? We could find data points like, there have been too many REFRESHes in the past 1 hour etc.
http://gerrit.cloudera.org:8080/#/c/8529/2/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/2/fe/src/main/java/org/apache/impala/util/TopNCache.java@41
PS2, Line 41: R extends Long>
Do we need this? It is almost always Long, no?
http://gerrit.cloudera.org:8080/#/c/8529/2/www/catalog.tmpl
File www/catalog.tmpl:
http://gerrit.cloudera.org:8080/#/c/8529/2/www/catalog.tmpl@23
PS2, Line 23: {{?has_large_tables}}
Like I mentioned elsewhere, my feeling is that these belong to metrics and should be exposed in /metrics?json for clients (like CM) to poll and chart.
--
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: 2
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Dec 2017 21:24:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/8529 )
Change subject: IMPALA-4886: Expose table metrics in the catalog web UI.
......................................................................
Patch Set 4: Code-Review+1
(5 comments)
Please raise jira for the remaining to-do tasks (around metrics/observability) when you get a chance. They'd make nice ramp-up/newbie tasks.
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
> The URL for table metrics already has a "?", so I am not sure how the ?json
Yea could be wrong, I haven't tried it myself, sorry for the wrong pointer.
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);
> Why abandon loading the rest of the /catalog page if the catalog usage call
My bad, I thought a single error breaks the whole page.
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@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();
> includeFileDesc is always true when this is called in the catalogd. For the
Agree.
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: metrics_.addTimer(ALTER_DURATION_METRIC);
> Hm, not sure if we need to add this check to every public function of table
Fine by me.
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
> That may not be a good idea since we will be accessing a table field in an
Ah nice catch.
--
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: 4
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 16 Dec 2017 05:42:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8529 )
Change subject: IMPALA-4886: Expose table metrics in the catalog web UI.
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1755/
--
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: 5
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Jan 2018 23:28:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
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 <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 16 Dec 2017 02:07:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8529 )
Change subject: IMPALA-4886: Expose table metrics in the catalog web UI.
......................................................................
Patch Set 6: Verified+1
--
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: 6
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Jan 2018 09:25:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Hello Philip Zeyliger, Vuk Ercegovac,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8529
to look at the new patch set (#2).
Change subject: [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
......................................................................
[PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
The following changes are included in this commit:
* Adds a lightweight framework for registering metrics in the JVM.
* Adds table-level metrics and enables these metrics to be exposed
through the catalog web UI.
* Adds a CatalogUsageMonitor class that monitors and reports the catalog
usage in terms of the tables with the highest memory requirements and
the tables with the highest number of metadata operations. The catalog
usage information is exposed in the /catalog page of the catalog web UI.
Change-Id: I37d407979e6d3b1a444b6b6265900b148facde9e
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M common/thrift/CatalogObjects.thrift
M common/thrift/Frontend.thrift
M common/thrift/JniCatalog.thrift
M fe/pom.xml
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java
M fe/src/main/java/org/apache/impala/catalog/HBaseTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
A fe/src/main/java/org/apache/impala/common/Metrics.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/util/TopNCache.java
A fe/src/test/java/org/apache/impala/util/TestTopNCache.java
M www/catalog.tmpl
A www/table_metrics.tmpl
23 files changed, 1,104 insertions(+), 76 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/8529/2
--
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: newpatchset
Gerrit-Change-Id: I37d407979e6d3b1a444b6b6265900b148facde9e
Gerrit-Change-Number: 8529
Gerrit-PatchSet: 2
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
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 1:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/8529/1/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
http://gerrit.cloudera.org:8080/#/c/8529/1/be/src/catalog/catalog-server.cc@391
PS1, Line 391: DCHECK_EQ(catalog_usage_result.large_tables.size(),
: catalog_usage_result.memory_estimates.size());
: DCHECK_EQ(catalog_usage_result.frequent_tables.size(),
: catalog_usage_result.num_metadata_operations.size());
> can be removed if using a struct instead of multiple arrays
Done
http://gerrit.cloudera.org:8080/#/c/8529/1/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
http://gerrit.cloudera.org:8080/#/c/8529/1/common/thrift/JniCatalog.thrift@602
PS1, Line 602: 1: required list<string> large_tables
> Is the idea that len(large_tables)==len(memory_estimates), and likewise len
Done
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/pom.xml
File fe/pom.xml:
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/pom.xml@365
PS1, Line 365: <artifactId>metrics-core</artifactId>
> Thanks. This is the right thing to use in my experience.
Good to hear :)
http://gerrit.cloudera.org:8080/#/c/8529/1/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/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1091
PS1, Line 1091: REFRESH
> why not sync this name to RELOAD?
Good point. However, I wanted the name to correspond to the name of the high level operation performed which is REFRESH. I agree that the naming may not be very accurate in this function.
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java
File fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java:
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java@27
PS1, Line 27: Sigleton
> nit(spelling): Singleton
Done
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java@36
PS1, Line 36: // TODO: Consider making it a configurable parameter.
> A somewhat cheap way to do this is to use:
I like this idea. Done
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java@48
PS1, Line 48: true
> why always evict for this one?
I just wanted a cheap way to make sure that frequently accessed tables from the past didn't prevent newly accessed tables from ever being inserted in the cache. A more elaborate schemes could be time-based rank reduction or something like that.
http://gerrit.cloudera.org:8080/#/c/8529/1/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/1/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@923
PS1, Line 923: HdfsTable.StorageStats stats,
: Reference<Boolean> hasIncrementalStats
> this is a surprising api since it modifies the last two args and does a bit
Done
http://gerrit.cloudera.org:8080/#/c/8529/1/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/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@197
PS1, Line 197: al stats
> this seems to only be written, not read. can it be removed or will it be us
It's read in L2248.
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@220
PS1, Line 220: table or partition
: // level.
> unclear what this is trying to say. is this: "aggregated table wide at the
It means that an instance of this class can be used for stats aggregated at the table or partition level. Reworked the comment a bit. Let me know if it's clear now.
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@256
PS1, Line 256: from
> nit: by
Done
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1735
PS1, Line 1735: hasIncrementalStats.getRef()
> there's a lot going on here-- surprising to see that its tested given that
Code changed a bit. Let me know if it's better now.
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1737
PS1, Line 1737: hasIncrementalStats_
> so incremental stats is a table-wide property and stored per partition? so
Yes, so the 'hasIncrementalStats_' field simply indicates that some (not necessarily all) partitions have incremental stats. The answer to the last question is yes. Say you create a table, add 10 partitions and run incremental stats. Then you add 10 more partitions. Until you run incremental stats again, those last partitions will not have incremental stats stored.
http://gerrit.cloudera.org:8080/#/c/8529/1/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/1/fe/src/main/java/org/apache/impala/util/TopNCache.java@35
PS1, Line 35: Always evict policy
> why is this needed? is it to reflect some sort of recency?
Yeah, see the comment in CatalogUsageMonitor.
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/util/TopNCache.java@94
PS1, Line 94: .
> do you want to enforce a sort order here or at the caller? might be simpler
I don't think there is a need to enforce a sort order. The UI allows you to sort on any column (table, name, metric value, etc).
http://gerrit.cloudera.org:8080/#/c/8529/1/www/catalog.tmpl
File www/catalog.tmpl:
http://gerrit.cloudera.org:8080/#/c/8529/1/www/catalog.tmpl@26
PS1, Line 26: Top-25
> if parameterizing N, this would need to change as well. perhaps omit the N?
Done
http://gerrit.cloudera.org:8080/#/c/8529/1/www/catalog.tmpl@34
PS1, Line 34:
> several ws issues here.
Done
http://gerrit.cloudera.org:8080/#/c/8529/1/www/catalog.tmpl@65
PS1, Line 65: Top-25
> same here
Done
http://gerrit.cloudera.org:8080/#/c/8529/1/www/catalog.tmpl@78
PS1, Line 78: {{#frequent_tables}}
> are these sorted (desc) by num operations? the screenshot for this one is n
No they are not. Using the UI you can sort based on that metric or the table name.
--
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: 1
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Nov 2017 20:43:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8529 )
Change subject: IMPALA-4886: Expose table metrics in the catalog web UI.
......................................................................
IMPALA-4886: Expose table metrics in the catalog web UI.
The following changes are included in this commit:
* Adds a lightweight framework for registering metrics in the JVM.
* Adds table-level metrics and enables these metrics to be exposed
through the catalog web UI.
* Adds a CatalogUsageMonitor class that monitors and reports the catalog
usage in terms of the tables with the highest memory requirements and
the tables with the highest number of metadata operations. The catalog
usage information is exposed in the /catalog page of the catalog web UI.
Change-Id: I37d407979e6d3b1a444b6b6265900b148facde9e
Reviewed-on: http://gerrit.cloudera.org:8080/8529
Reviewed-by: Dimitris Tsirogiannis <dt...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M common/thrift/CatalogObjects.thrift
M common/thrift/Frontend.thrift
M common/thrift/JniCatalog.thrift
M fe/pom.xml
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java
M fe/src/main/java/org/apache/impala/catalog/HBaseTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
A fe/src/main/java/org/apache/impala/common/Metrics.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/main/java/org/apache/impala/util/TopNCache.java
A fe/src/test/java/org/apache/impala/util/TestTopNCache.java
M tests/webserver/test_web_pages.py
M www/catalog.tmpl
A www/table_metrics.tmpl
24 files changed, 1,206 insertions(+), 113 deletions(-)
Approvals:
Dimitris Tsirogiannis: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: I37d407979e6d3b1a444b6b6265900b148facde9e
Gerrit-Change-Number: 8529
Gerrit-PatchSet: 7
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger 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 1:
(3 comments)
I took a very brief look. I was worried when you said "lightweight framework", but it turns out you're using codahale/yammer metrics, which is the right thing to do.
You know this, but TopNCache doesn't have a unit test.
I didn't look at all of this.
http://gerrit.cloudera.org:8080/#/c/8529/1/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
http://gerrit.cloudera.org:8080/#/c/8529/1/common/thrift/JniCatalog.thrift@602
PS1, Line 602: 1: required list<string> large_tables
Is the idea that len(large_tables)==len(memory_estimates), and likewise len(frequent_table)=len(num_metadata_operations)? I think it'd be more honest for this to be "list<LargeTable> large_tables", with LargeTable being a struct that has a tablename and details about that table that you want to share.
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/pom.xml
File fe/pom.xml:
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/pom.xml@365
PS1, Line 365: <artifactId>metrics-core</artifactId>
Thanks. This is the right thing to use in my experience.
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java
File fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java:
http://gerrit.cloudera.org:8080/#/c/8529/1/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java@36
PS1, Line 36: // TODO: Consider making it a configurable parameter.
A somewhat cheap way to do this is to use:
Integer.getInteger("org.apache.impala.catalog.CatalogUsageMonitor.NUM_TABLES_TRACKED", 25)
here.
That gets the number from system properties. It's pretty weak configuration, but I've used it to nice effect for constants that really should be fine as is.
--
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: 1
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 Nov 2017 20:00:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4886: Expose table metrics in the catalog web UI.
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8529 )
Change subject: IMPALA-4886: Expose table metrics in the catalog web UI.
......................................................................
Patch Set 6: Code-Review+2
Rebase and retry gvo due to failed flaky test (IMPALA-6092).
--
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: 6
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Jan 2018 05:44:49 +0000
Gerrit-HasComments: No