Tim Armstrong has posted comments on this change. Change subject: IMPALA-4024: Add "system" database and expose Impala metrics as a table ......................................................................
Patch Set 17: (16 comments) Focused on the fe part. I think we need to sort out the TableId thing, otherwise mostly minor comments. http://gerrit.cloudera.org:8080/#/c/3863/17/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: Line 360: Extra line. http://gerrit.cloudera.org:8080/#/c/3863/17/common/thrift/StatestoreService.thrift File common/thrift/StatestoreService.thrift: PS17, Line 67: sContaine List? 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: // For SELECT, we should always check auth policy. I don't think this comment is necessary here - that logic is implemented in checkSystemDbAccess() Line 527: * Return false if we need further authorization check. "if authorization should be checked in the usual way." Line 538: return false; Add a comment like: // Check authorization for SELECT on system tables in the usual way. 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) && !(table instanceof SystemTable)) This doesn't seem right - we are referencing the system table and need to send the descriptor to the backend. Am I missing something? This is probably the wrong way to fix the problem. 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't set up the backend descriptor table correctly when there are multiple tables per fragment. I suspect if you joined two system tables you would run into problems. It looks like automatically assigned table ids start at 0. Maybe we should assign hardcoded negative table ids to each system table. I.e. define constants in TableId like SYSTEM_METRICS = -1, etc. 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: "System database for Impala cluster, including impala metrics currently"; Remove "including impala metrics currently" so we don't have to remember to update the comment. 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: numRows_ Do we set numRows_? I think we want to set it to something at least approximate so that the planner knows its a smallish table and has something to work with. 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 70: // private void computeSingleScanRangeLocations(Analyzer analyzer) { Old code Line 80: * @throws InternalException I don't think we need the throws annotation. Line 82: Extra line. Line 123: // tblDescripter Remove comment? 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 for consistency. 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: (no SELECT permissions) Remove- we do have select permissions. Line 2175: AuthzOk("select * from system.metrics"); We're missing a negative test where we don't have access to a system table. Maybe we could add a fake table to ImpaladTestCatalog? Could you also add a AuthzError test for selecting from a non-existent system table. -- 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: 17 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Kathy Sun <[email protected]> Gerrit-Reviewer: Kathy Sun <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
