Kathy Sun has posted comments on this change. Change subject: IMPALA-4024: Add "system" database and expose Impala metrics as a table ......................................................................
Patch Set 20: (25 comments) > We also need to fix some of the describe commands: > > [tarmstrong-box.ca.cloudera.com:21000] > show create table metrics; > Query: show create table metrics > Query submitted at: 2016-09-07 16:53:20 (Coordinator: > http://0.0.0.0:25000) > ERROR: NullPointerException: null > > [tarmstrong-box.ca.cloudera.com:21000] > describe formatted > metrics; > Query: describe formatted metrics > Query submitted at: 2016-09-07 16:53:40 (Coordinator: > http://0.0.0.0:25000) > ERROR: NullPointerException: null not include in the 21 patch, discuss offline discuss offline http://gerrit.cloudera.org:8080/#/c/3863/20/be/src/exec/system-table-scan-node.cc File be/src/exec/system-table-scan-node.cc: Line 54: DCHECK(table_name_ == TSystemTableName::type::METRICS); > We prefer DCHECK_EQ Done Line 76: case TSystemTableName::QUERIES: > We can remove this case (we already check that it's only METRICS above). Th Done Line 89: scanner_->MaterializeNextTuple(tuple_pool, tuple, tuple_desc_); > We need to check the return status here. Done Line 127: ++(scanner_->next_row_idx_); > MaterializeNextTuple() should increment this inside the scanner. Done Line 128: scanner_->CheckEOS(); > This doesn't seem too necessary? Done http://gerrit.cloudera.org:8080/#/c/3863/20/be/src/exec/system-table-scan-node.h File be/src/exec/system-table-scan-node.h: Line 31: class SystemTableScanNode : public ScanNode { > I think we should add a short class comment. E.g. Done Line 37: /// Create schema and columns to slots mapping. > Extra space before schema. Done Line 40: /// Start scan. > Extra space before scan. Done PS20, Line 43: metrics > from the system table. Done PS20, Line 69: next_row_idx_ > next_row_idx was removed. Done http://gerrit.cloudera.org:8080/#/c/3863/20/be/src/exec/system-table-scanner.cc File be/src/exec/system-table-scanner.cc: Line 38: "SystemTableScanNode::$0() failed to allocate " > The string should fit one one line. Done PS20, Line 107: group_name > Should be metric_name. Done Line 116: return Status(Substitute("column error")); > This shouldn't be possible, so let's make this a DCHECK. E.g. Done http://gerrit.cloudera.org:8080/#/c/3863/20/be/src/exec/system-table-scanner.h File be/src/exec/system-table-scanner.h: Line 47: virtual void CheckEOS() = 0; > I don't think this is necessary - MaterializeNextTuple should set eos_ Done Line 50: bool eos_; > Let's make eos_ private and expose it with an accessor function. Done Line 53: int next_row_idx_; > I think this can live in the MetricScanner subclass for now. Done Line 73: if (next_row_idx_ >= metric_pool_.size()) eos_ = true; > We can just do this at the end of MaterializeNextTuple() Done http://gerrit.cloudera.org:8080/#/c/3863/20/fe/src/main/java/com/cloudera/impala/analysis/TupleDescriptor.java File fe/src/main/java/com/cloudera/impala/analysis/TupleDescriptor.java: Line 205: Table table = getTable(); > You can undo this change - it doesn't look needed. Done http://gerrit.cloudera.org:8080/#/c/3863/20/fe/src/main/java/com/cloudera/impala/catalog/SystemDb.java File fe/src/main/java/com/cloudera/impala/catalog/SystemDb.java: PS20, Line 32: matrics > /// Table for all Impala daemon metrics. Done http://gerrit.cloudera.org:8080/#/c/3863/20/fe/src/main/java/com/cloudera/impala/catalog/SystemTable.java File fe/src/main/java/com/cloudera/impala/catalog/SystemTable.java: PS20, Line 66: info schema > system table. Done http://gerrit.cloudera.org:8080/#/c/3863/20/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java File fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java: PS20, Line 2188: .getDb("system") > Can we use getSystemDb() here? Done Line 2189: Table table= new SystemTable( > Missing space before = Done Line 2218: // table doesn't exist > This comment "table doesn't exist" doesn't seem necessary. Done Line 2258: Db system = catalog.getDb("system"); > Can we use getSystemDb() here? Done Line 2259: Table table= new SystemTable( > Need space before = Done -- 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: 20 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
