Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201609021042.u82AgT5x027638@ip-10-146-233-104.ec2.internal>
Date: Fri, 2 Sep 2016 10:42:29 +0000
From: "Kathy Sun (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, dev@impala.incubator.apache.org
CC: Tim Armstrong <tarmstrong@cloudera.com>
Reply-To: kathy.sun@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4024=3A_Add_=22system=22_database_and_expose_Impala_metrics_as_a_table=0A?=
In-Reply-To: <gerrit.1470691807000.I7adbeb45220c468e43b424d70c30b952f6cec2cd@gerrit.cloudera.org>
References: <gerrit.1470691807000.I7adbeb45220c468e43b424d70c30b952f6cec2cd@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Fri, 02 Sep 2016 10:42:35 -0000

Kathy Sun has posted comments on this change.

Change subject: IMPALA-4024: Add "system" database and expose Impala metrics as a table
......................................................................


Patch Set 18:

(15 comments)

made change based on review.
1. assign table id to metrics table to make sure table descriptor i
2. add negative tests in authz test, also add test case for admin user.
3. add test for "show databases", check the existence of "system" database
3. small changes to further separate metrics related code out.

http://gerrit.cloudera.org:8080/#/c/3863/17/common/thrift/CatalogObjects.thrift
File common/thrift/CatalogObjects.thrift:

Line 360: }
> Extra line.
Done


http://gerrit.cloudera.org:8080/#/c/3863/17/common/thrift/StatestoreService.thrift
File common/thrift/StatestoreService.thrift:

PS17, Line 67: sList {
> List?
Done


http://gerrit.cloudera.org:8080/#/c/3863/17/fe/src/main/java/com/cloudera/impala/analysis/AnalysisContext.java
File fe/src/main/java/com/cloudera/impala/analysis/AnalysisContext.java:

Line 469:     if (dbName != null && checkSystemDbAccess(dbName, request.getPrivilege())) {
> I don't think this comment is necessary here - that logic is implemented in
Done


Line 527:    */
> "if authorization should be checked in the usual way."
Done


Line 538:           return false;
> Add a comment like:
Done


http://gerrit.cloudera.org:8080/#/c/3863/17/fe/src/main/java/com/cloudera/impala/analysis/DescriptorTable.java
File fe/src/main/java/com/cloudera/impala/analysis/DescriptorTable.java:

Line 169:         if (table != null && !(table instanceof View))
> This doesn't seem right - we are referencing the system table and need to s
discuss offline


http://gerrit.cloudera.org:8080/#/c/3863/17/fe/src/main/java/com/cloudera/impala/analysis/TupleDescriptor.java
File fe/src/main/java/com/cloudera/impala/analysis/TupleDescriptor.java:

Line 204:     if (getTable() != null && !(getTable() instanceof View)) {
> I think we do need to assign a table id for system tables, otherwise we won
discuss offline


http://gerrit.cloudera.org:8080/#/c/3863/17/fe/src/main/java/com/cloudera/impala/catalog/SystemDb.java
File fe/src/main/java/com/cloudera/impala/catalog/SystemDb.java:

Line 35:   private void AddMetricTable() {
> Remove "including impala metrics currently" so we don't have to remember to
Done


http://gerrit.cloudera.org:8080/#/c/3863/17/fe/src/main/java/com/cloudera/impala/catalog/SystemTable.java
File fe/src/main/java/com/cloudera/impala/catalog/SystemTable.java:

PS17, Line 110: 
> Do we set numRows_? I think we want to set it to something at least approxi
discussed offline


http://gerrit.cloudera.org:8080/#/c/3863/17/fe/src/main/java/com/cloudera/impala/planner/SystemTableScanNode.java
File fe/src/main/java/com/cloudera/impala/planner/SystemTableScanNode.java:

Line 80: 
> I don't think we need the throws annotation.
Done


Line 82:       scanRangeLocations.scan_range = new TScanRange();
> Extra line.
Done


Line 123:       String prefix, String detailPrefix, TExplainLevel detailLevel) {
> Remove comment?
Done


http://gerrit.cloudera.org:8080/#/c/3863/17/fe/src/main/java/com/cloudera/impala/service/FeSupport.java
File fe/src/main/java/com/cloudera/impala/service/FeSupport.java:

PS17, Line 93: NativeGetBackends
> We seem to use "Backends" as the capitalization throughout. Let's do that f
Done


http://gerrit.cloudera.org:8080/#/c/3863/17/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
File fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java:

PS17, Line 105: pestiny')
> Remove- we do have select permissions.
Oops... I was about to say no INSERT permission…
removed.


Line 2175:   }
> We're missing a negative test where we don't have access to a system table.
discuss offline


-- 
To view, visit http://gerrit.cloudera.org:8080/3863
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7adbeb45220c468e43b424d70c30b952f6cec2cd
Gerrit-PatchSet: 18
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <kathy.sun@cloudera.com>
Gerrit-Reviewer: Kathy Sun <kathy.sun@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes