Tim Armstrong has posted comments on this change. Change subject: IMPALA-4024: Add "system" database and expose Impala metrics as a table ......................................................................
Patch Set 20: (27 comments) I think we can make a lot of small improvements but we're converging :) http://gerrit.cloudera.org:8080/#/c/3863/18/be/src/exec/system-table-scan-node.cc File be/src/exec/system-table-scan-node.cc: PS18, Line 49: tuple_id_( > Done. Looks like dead/old code in data-source-scan-node. It should be removed there too - feel free to create a patch to fix it there if you have any free time. http://gerrit.cloudera.org:8080/#/c/3863/19/be/src/exec/system-table-scan-node.cc File be/src/exec/system-table-scan-node.cc: Line 93: Status SystemTableScanNode::GetNext(RuntimeState* state, RowBatch* row_batch, bool* eos) { Need to check the return status here. 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 Line 76: case TSystemTableName::QUERIES: We can remove this case (we already check that it's only METRICS above). This will be easy to add back in later. Line 89: scanner_->MaterializeNextTuple(tuple_pool, tuple, tuple_desc_); We need to check the return status here. Line 127: ++(scanner_->next_row_idx_); MaterializeNextTuple() should increment this inside the scanner. Line 128: scanner_->CheckEOS(); This doesn't seem too necessary? 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. /// A scan node that exposes Impala system state as a table. /// /// Different SystemTableScanner subclasses gather data from different Impala subsystems and /// materialize it into Tuples. Line 37: /// Create schema and columns to slots mapping. Extra space before schema. Line 40: /// Start scan. Extra space before scan. PS20, Line 43: metrics from the system table. PS20, Line 69: next_row_idx_ next_row_idx was removed. http://gerrit.cloudera.org:8080/#/c/3863/19/be/src/exec/system-table-scanner.cc File be/src/exec/system-table-scanner.cc: Line 63: enum MetricsColumn { Let's put this in the MetricsScanner class in the header. 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. Let's also add an empty line before and after. Line 116: return Status(Substitute("column error")); This shouldn't be possible, so let's make this a DCHECK. E.g. DCHECK(false) << "Unknown column position " << slot_desc->col_pos(); 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_ Line 50: bool eos_; Let's make eos_ private and expose it with an accessor function. Line 53: int next_row_idx_; I think this can live in the MetricScanner subclass for now. Line 73: if (next_row_idx_ >= metric_pool_.size()) eos_ = true; We can just do this at the end of MaterializeNextTuple() 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. 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. 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. 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? Line 2189: Table table= new SystemTable( Missing space before = Line 2218: // table doesn't exist This comment "table doesn't exist" doesn't seem necessary. Line 2258: Db system = catalog.getDb("system"); Can we use getSystemDb() here? Line 2259: Table table= new SystemTable( Need space before = -- 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
