Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/10671 )
Change subject: IMPALA-6835: Improve Kudu scanner error messages to include the table name and the plan node id ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/10671/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10671/1//COMMIT_MSG@7 PS1, Line 7: IMPALA-6835: Improve Kudu scanner error messages to include the table name and the plan node id nit: it would be nice to find a shorter title without loosing too much information - e.g. "Add table name and node id to Kudu scanner errors" http://gerrit.cloudera.org:8080/#/c/10671/1//COMMIT_MSG@11 PS1, Line 11: TPlanNode id which made it inconveient to debug. This change add nit: missing s http://gerrit.cloudera.org:8080/#/c/10671/1/be/src/exec/kudu-scanner.cc File be/src/exec/kudu-scanner.cc: http://gerrit.cloudera.org:8080/#/c/10671/1/be/src/exec/kudu-scanner.cc@151 PS1, Line 151: Substitute("Unable to deserialize scan token for PlanNode '$0' with KuduTable '$1'", : scan_node_->id(), scan_node_->table_->name())); This made me think about ways to make it less verbose - a function like "string KuduScanner::AppendInfo(const char* msg)" could be created to add the node id and table name to a message. It is up to you, but I think that it's worth to add +1 function to make the code a bit shorter and ensure consistent messages. http://gerrit.cloudera.org:8080/#/c/10671/1/be/src/exec/kudu-scanner.cc@167 PS1, Line 167: scan_node_->id() I am not sure about this, but I would add table name for these errors too. http://gerrit.cloudera.org:8080/#/c/10671/1/be/src/exec/kudu-scanner.cc@168 PS1, Line 168: VLOG_ROW << "Starting KuduScanner with ReadMode=" << mode << " timeout=" << It may be useful do add the same info to logs. -- To view, visit http://gerrit.cloudera.org:8080/10671 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0377fc8591738dc45092d228fcf292ddbb367825 Gerrit-Change-Number: 10671 Gerrit-PatchSet: 1 Gerrit-Owner: Pooja Nilangekar <pooja.nilange...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Pooja Nilangekar <pooja.nilange...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 11 Jun 2018 17:44:52 +0000 Gerrit-HasComments: Yes
