Tim Armstrong 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: (3 comments) Just some minor comments here. 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: PlanNode I don't think we use the "PlanNode" terminology in user-facing error messages yet. In other places we say things like "node with id 10", so would be better to be consistent with that. http://gerrit.cloudera.org:8080/#/c/10671/1/be/src/exec/kudu-scanner.cc@151 PS1, Line 151: KuduTable Maybe "Kudu table"? That seems more human readable. http://gerrit.cloudera.org:8080/#/c/10671/1/be/src/exec/kudu-scanner.cc@245 PS1, Line 245: Substitute("Unable to open scanner for PlanNode '$0' with KuduTable '$1'", We're depending on the fact that KUDU_RETURN_IF_ERROR lazily evaluates it's second argument, right? Otherwise we'd be constructing a lot of unnecessary strings. If so, can you add a comment to KUDU_RETURN_IF_ERROR to mention this behaviour, since it's something that other code depends on. -- 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 19:00:16 +0000 Gerrit-HasComments: Yes
