Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201711142000.vAEK0s8N002289@ip-10-146-233-104.ec2.internal>
Date: Tue, 14 Nov 2017 20:00:54 +0000
From: "Philip Zeyliger (Code Review)" <gerrit@cloudera.org>
To: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_=5BPREVIEW=5D_IMPALA-4886=3A_Expose_table_metrics_in_the_catalog_web_UI=2E=0A?=
In-Reply-To: <gerrit.1510601400000.I37d407979e6d3b1a444b6b6265900b148facde9e@gerrit.cloudera.org>
References: <gerrit.1510601400000.I37d407979e6d3b1a444b6b6265900b148facde9e@gerrit.cloudera.org>
Reply-To: philip@cloudera.com, impala-cr@cloudera.com, marcelk@gmail.com,
        reviews@impala.incubator.apache.org, dtsirogiannis@cloudera.com
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="crHWv+nlY7c="; charset=UTF-8
archived-at: Tue, 14 Nov 2017 20:01:01 -0000

--crHWv+nlY7c=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Philip Zeyliger has posted comments on this change=2E ( http://gerrit=2Eclo=
udera=2Eorg:8080/8529 )

Change subject: [PREVIEW] IMPALA-4886: Expose tabl=
e metrics in the catalog web UI=2E
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E


Patch Set 1:

(3 comments)

I took a very brief loo=
k=2E 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=2E

Yo=
u know this, but TopNCache doesn't have a unit test=2E

I didn't look at al=
l of this=2E

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8529/1/common/thrift/=
JniCatalog=2Ethrift
File common/thrift/JniCatalog=2Ethrift:

http://gerrit=
=2Ecloudera=2Eorg:8080/#/c/8529/1/common/thrift/JniCatalog=2Ethrift@602
PS1=
, Line 602:   1: required list<string> large_tables
Is the idea that len(la=
rge_tables)=3D=3Dlen(memory_estimates), and likewise len(frequent_table)=3D=
len(num_metadata_operations)? I think it'd be more honest for this to be "l=
ist<LargeTable> large_tables", with LargeTable being a struct that has a ta=
blename and details about that table that you want to share=2E


http://ger=
rit=2Ecloudera=2Eorg:8080/#/c/8529/1/fe/pom=2Exml
File fe/pom=2Exml:

http:=
//gerrit=2Ecloudera=2Eorg:8080/#/c/8529/1/fe/pom=2Exml@365
PS1, Line 365:  =
     <artifactId>metrics-core</artifactId>
Thanks=2E This is the right thin=
g to use in my experience=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8529=
/1/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor=2Ejava
Fi=
le fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor=2Ejava:

=
http://gerrit=2Ecloudera=2Eorg:8080/#/c/8529/1/fe/src/main/java/org/apache/=
impala/catalog/CatalogUsageMonitor=2Ejava@36
PS1, Line 36:   // TODO: Consi=
der making it a configurable parameter=2E
A somewhat cheap way to do this i=
s to use:

Integer=2EgetInteger("org=2Eapache=2Eimpala=2Ecatalog=2ECatalogU=
sageMonitor=2ENUM_TABLES_TRACKED", 25)

here=2E

That gets the number from =
system properties=2E It's pretty weak configuration, but I've used it to ni=
ce effect for constants that really should be fine as is=2E


-- 
To view,=
 visit http://gerrit=2Ecloudera=2Eorg:8080/8529
To unsubscribe, visit http:=
//gerrit=2Ecloudera=2Eorg:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-=
Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I37d407979e6d3=
b1a444b6b6265900b148facde9e
Gerrit-Change-Number: 8529
Gerrit-PatchSet: 1
G=
errit-Owner: Dimitris Tsirogiannis <dtsirogiannis@cloudera=2Ecom>
Gerrit-Re=
viewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera=2Ecom>
Gerrit-Reviewe=
r: Philip Zeyliger <philip@cloudera=2Ecom>
Gerrit-Comment-Date: Tue, 14 Nov=
 2017 20:00:54 +0000
Gerrit-HasComments: Yes

--crHWv+nlY7c=--