impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kathy Sun (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4024: Add "system" database and expose Impala metrics as a table
Date Thu, 08 Sep 2016 19:36:06 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 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.sun@cloudera.com>
Gerrit-Reviewer: Kathy Sun <kathy.sun@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message