impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kathy Sun (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4024: Add "system" database and expose Impala metrics as a table
Date Fri, 02 Sep 2016 10:42:29 GMT
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:


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.
File common/thrift/CatalogObjects.thrift:

Line 360: }
> Extra line.
File common/thrift/StatestoreService.thrift:

PS17, Line 67: sList {
> List?
File fe/src/main/java/com/cloudera/impala/analysis/

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

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

Line 538:           return false;
> Add a comment like:
File fe/src/main/java/com/cloudera/impala/analysis/

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
File fe/src/main/java/com/cloudera/impala/analysis/

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
File fe/src/main/java/com/cloudera/impala/catalog/

Line 35:   private void AddMetricTable() {
> Remove "including impala metrics currently" so we don't have to remember to
File fe/src/main/java/com/cloudera/impala/catalog/

PS17, Line 110: 
> Do we set numRows_? I think we want to set it to something at least approxi
discussed offline
File fe/src/main/java/com/cloudera/impala/planner/

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

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

Line 123:       String prefix, String detailPrefix, TExplainLevel detailLevel) {
> Remove comment?
File fe/src/main/java/com/cloudera/impala/service/

PS17, Line 93: NativeGetBackends
> We seem to use "Backends" as the capitalization throughout. Let's do that f
File fe/src/test/java/com/cloudera/impala/analysis/

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

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7adbeb45220c468e43b424d70c30b952f6cec2cd
Gerrit-PatchSet: 18
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <>
Gerrit-Reviewer: Kathy Sun <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message