[Impala-ASF-CR] IMPALA-6835: Improve Kudu scanner error messages to include the table name and the plan node id

2018-06-11 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins 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: Verified+1


--
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 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Pooja Nilangekar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 11 Jun 2018 20:09:11 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6835: Improve Kudu scanner error messages to include the table name and the plan node id

2018-06-11 Thread Tim Armstrong (Code Review) 
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 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Pooja Nilangekar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 11 Jun 2018 19:00:16 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6835: Improve Kudu scanner error messages to include the table name and the plan node id

2018-06-11 Thread Csaba Ringhofer (Code Review) 
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 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Pooja Nilangekar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 11 Jun 2018 17:44:52 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6835: Improve Kudu scanner error messages to include the table name and the plan node id

2018-06-11 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins 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:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2631/


--
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 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Pooja Nilangekar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 11 Jun 2018 16:53:20 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6835: Improve Kudu scanner error messages to include the table name and the plan node id

2018-06-08 Thread Pooja Nilangekar (Code Review) 
Pooja Nilangekar has uploaded this change for review. ( 
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
..

IMPALA-6835: Improve Kudu scanner error messages to include the table name and 
the plan node id

Previously, the error messages in KuduScanner only contained the
reason for failure. They did not contain the KuduTable name or the
TPlanNode id which made it inconveient to debug. This change add
the TPlanNode id to all error messages and the KuduTable name
whenever applicable.

This change was manually tested by explicitly returning failure
while scanning kudu tables.

Change-Id: I0377fc8591738dc45092d228fcf292ddbb367825
---
M be/src/exec/kudu-scanner.cc
1 file changed, 24 insertions(+), 12 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/10671/1
--
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: newchange
Gerrit-Change-Id: I0377fc8591738dc45092d228fcf292ddbb367825
Gerrit-Change-Number: 10671
Gerrit-PatchSet: 1
Gerrit-Owner: Pooja Nilangekar