impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <>
Subject [Impala-ASF-CR] [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.
Date Wed, 13 Dec 2017 22:34:50 GMT
Bharath Vissapragada has posted comments on this change. (

Change subject: [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.

Patch Set 3:


The patch mostly lgtm. I have some minor suggestions before I can +1.
Commit Message:
PS3, Line 7: [PREVIEW]
File be/src/catalog/
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.
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());
File be/src/catalog/
PS3, Line 359:   GetCatalogUsage(document);
Check the return value and return early if it threw a failure?

if (!GetCatalogUsage().ok()) return;
PS3, Line 516: object_name
File be/src/catalog/catalog.h:
PS2, Line 96:   Status GetCatalogUsage(TGetCatalogUsageResponse* response);
> I renamed the other one to GetCatalogUsage. I thought of keeping it a bit m
File fe/src/main/java/org/apache/impala/catalog/
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?
File fe/src/main/java/org/apache/impala/catalog/
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.
File fe/src/main/java/org/apache/impala/catalog/
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).
File fe/src/main/java/org/apache/impala/catalog/
PS3, Line 196:  
..any of the partitions in.. ?
PS3, Line 1720: metadataSizeEstimate
may be memUsageEstimate to be inline with CatalogUsage..terminology?
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
File fe/src/main/java/org/apache/impala/catalog/
PS3, Line 151:   public Metrics getMetrics() { return metrics_; }
File fe/src/main/java/org/apache/impala/common/
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.
File fe/src/main/java/org/apache/impala/service/
PS3, Line 362:     final Timer.Context context
Include the lock in the context?
PS3, Line 3334:    */
Maybe add a comment that this increments the opsCount, otherwise, some callers might call
it repeatedly resulting in spurious counts.
File fe/src/main/java/org/apache/impala/util/
PS3, Line 56:     function_ = f;
Preconditions.checkState(maxCapacity > 0)?
File fe/src/test/java/org/apache/impala/util/
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.
PS3, Line 52: 
Also assert the ranking by randomizing the puts?
File www/catalog.tmpl:
PS3, Line 135:           <td><a href="table_metrics?name={{fqtn}}">{{name}}-metrics</a></td>
add some unit tests for this? (

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Vuk Ercegovac <>
Gerrit-Comment-Date: Wed, 13 Dec 2017 22:34:50 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message