Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201704141722.v3EHMkRO031430@ip-10-146-233-104.ec2.internal>
Date: Fri, 14 Apr 2017 17:22:46 +0000
From: "Sailesh Mukil (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Tim Armstrong <tarmstrong@cloudera.com>
Reply-To: sailesh@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5198=3A_Error_messages_are_sometimes_dropped_before_reaching_client=0A?=
In-Reply-To: <gerrit.1492104798000.I5d9d63610eb0d2acae3a9303ce46e1410727ce87@gerrit.cloudera.org>
References: <gerrit.1492104798000.I5d9d63610eb0d2acae3a9303ce46e1410727ce87@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Fri, 14 Apr 2017 17:22:55 -0000

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