impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Date Fri, 14 Apr 2017 17:22:46 GMT
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6627/1//COMMIT_MSG
Commit Message:

Line 7: IMPALA-5198: Error messages are sometimes dropped before reaching client
> Maybe describe the symptoms/bug more generally, so it's easier to understan
Done


http://gerrit.cloudera.org:8080/#/c/6627/1/be/src/service/impala-beeswax-server.cc
File be/src/service/impala-beeswax-server.cc:

Line 288:   if (exec_state->coord() != NULL) {
> Is this addition related to the change? I don't understand why we're printi
It's up for debate whether we want to add this or not. I added it to be consistent with the
HS2 server.
https://github.com/apache/incubator-impala/blob/6a9df540967e07b09524268d0cc52b7d10835676/be/src/service/impala-hs2-server.cc#L826

Technically GetLog()/get_log() is not just for retrieving just error logs, it can be used
at any point in the query to get any sort of information available. However, our use of it
in beeswax seems just to be for retrieving errors.

If we think this is unnecessary for beeswax, I can go ahead and remove it.


http://gerrit.cloudera.org:8080/#/c/6627/1/be/src/util/error-util.cc
File be/src/util/error-util.cc:

Line 123:     const ArgType& arg4, const ArgType& arg5, const ArgType& arg6, const
ArgType& arg7,
> It seems like conceptually the problem is the Status(TStatus) constructor s
I spent some time looking at the different places that TStatus is used, and although the best
option would be to make TStatus identical in structure to Status (it currently isn't), doing
so would require a lot of changes and cause inconsistencies between the different TStatus
objects (thrift::TStatus, TCLIService::TStatus, impala::TStatus). Some of these are exposed
to clients and would cause a breaking change.

So I decided to just move this unwrapping to the Status level by introducing a function called
Status::FromThrift(), which makes the wrapping and unrwrapping happen at the same level.


-- 
To view, visit http://gerrit.cloudera.org:8080/6627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message