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....@cloudera.com> Gerrit-Reviewer: Kathy Sun <kathy....@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
