You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2017/04/13 17:33:18 UTC
[Impala-ASF-CR] IMPALA-5198: test exchange delays custom cluster test failed on s3 tests
Sailesh Mukil has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6627
Change subject: IMPALA-5198: test_exchange_delays custom cluster test failed on s3 tests
......................................................................
IMPALA-5198: test_exchange_delays custom cluster test failed on s3 tests
The Status::ToThrift() function takes the ErrorMsg, and pushes both
the msg() and details() into the TStatus::error_msgs list.
However, when we unpack the TStatus object into a Status object, we
just copy all the TStatus::error_msgs to Status::ErrorMsg::details_
and leave Status::ErrorMsg::message_ blank.
This led to the error message not being printed in certain cases. The
beeswax server had code to comprimise for this setback, which is now
removed, and is similar in behavior to the HS2 server.
Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
---
M be/src/service/impala-beeswax-server.cc
M be/src/util/error-util.cc
M be/src/util/error-util.h
3 files changed, 13 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/6627/1
--
To view, visit http://gerrit.cloudera.org:8080/6627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
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 <sa...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 3:
(1 comment)
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: // Add warnings from analysis
> That's interesting, didn't realise that.
Sounds good. Done.
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6627
to look at the new patch set (#8).
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
IMPALA-5198: Error messages are sometimes dropped before reaching client
The Status::ToThrift() function takes the ErrorMsg, and pushes both
the msg() and details() into the TStatus::error_msgs list.
However, when we unpack the TStatus object into a Status object, we
just copy all the TStatus::error_msgs to Status::ErrorMsg::details_
and leave Status::ErrorMsg::message_ blank.
This led to the error message not being printed in certain cases which
is now fixed.
The PlanFragmentExecutor had some code to add query statuses to
the error_log (IMP-633), which is no longer necessary after a
future patch (IMPALA-762) explicitly returned the query status to
the client via get_log(), making the adding of the query statuses
to the error_log redundant. That code in the PFE has been removed
and a test has been added to make sure that the case it previously
tried to fix doesn't regress.
Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
---
M be/src/common/status.cc
M be/src/common/status.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/util/error-util.h
M tests/data_errors/test_data_errors.py
5 files changed, 51 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/6627/8
--
To view, visit http://gerrit.cloudera.org:8080/6627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6627
to look at the new patch set (#9).
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
IMPALA-5198: Error messages are sometimes dropped before reaching client
The Status::ToThrift() function takes the ErrorMsg, and pushes both
the msg() and details() into the TStatus::error_msgs list.
However, when we unpack the TStatus object into a Status object, we
just copy all the TStatus::error_msgs to Status::ErrorMsg::details_
and leave Status::ErrorMsg::message_ blank.
This led to the error message not being printed in certain cases which
is now fixed.
The PlanFragmentExecutor had some code to add query statuses to
the error_log (IMP-633), which is no longer necessary after a
future patch (IMPALA-762) explicitly returned the query status to
the client via get_log(), making the adding of the query statuses
to the error_log redundant. That code in the PFE has been removed
and a test has been added to make sure that the case it previously
tried to fix doesn't regress.
Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
---
M be/src/common/status.cc
M be/src/common/status.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/util/error-util.h
M tests/data_errors/test_data_errors.py
5 files changed, 51 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/6627/9
--
To view, visit http://gerrit.cloudera.org:8080/6627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6627/4/be/src/service/impala-beeswax-server.cc
File be/src/service/impala-beeswax-server.cc:
Line 290
do the details_ still show up somewhere? (Not arguing this is the right place for that, but just want to be sure the details don't get lost).
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6627
to look at the new patch set (#7).
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
IMPALA-5198: Error messages are sometimes dropped before reaching client
The Status::ToThrift() function takes the ErrorMsg, and pushes both
the msg() and details() into the TStatus::error_msgs list.
However, when we unpack the TStatus object into a Status object, we
just copy all the TStatus::error_msgs to Status::ErrorMsg::details_
and leave Status::ErrorMsg::message_ blank.
This led to the error message not being printed in certain cases. The
beeswax server had code to comprimise for this setback, which is now
removed.
Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
---
M be/src/common/status.cc
M be/src/common/status.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/util/error-util.h
4 files changed, 29 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/6627/7
--
To view, visit http://gerrit.cloudera.org:8080/6627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 9: Code-Review+2
(1 comment)
Rebase. Carry +2
http://gerrit.cloudera.org:8080/#/c/6627/8/tests/data_errors/test_data_errors.py
File tests/data_errors/test_data_errors.py:
PS8, Line 58: sel
> did you mean to use that?
Nope. Removed it.
--
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6627
to look at the new patch set (#6).
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
IMPALA-5198: Error messages are sometimes dropped before reaching client
The Status::ToThrift() function takes the ErrorMsg, and pushes both
the msg() and details() into the TStatus::error_msgs list.
However, when we unpack the TStatus object into a Status object, we
just copy all the TStatus::error_msgs to Status::ErrorMsg::details_
and leave Status::ErrorMsg::message_ blank.
This led to the error message not being printed in certain cases. The
beeswax server had code to comprimise for this setback, which is now
removed.
Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
---
M be/src/common/status.cc
M be/src/common/status.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/util/error-util.cc
M be/src/util/error-util.h
5 files changed, 41 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/6627/6
--
To view, visit http://gerrit.cloudera.org:8080/6627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#4).
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
IMPALA-5198: Error messages are sometimes dropped before reaching client
The Status::ToThrift() function takes the ErrorMsg, and pushes both
the msg() and details() into the TStatus::error_msgs list.
However, when we unpack the TStatus object into a Status object, we
just copy all the TStatus::error_msgs to Status::ErrorMsg::details_
and leave Status::ErrorMsg::message_ blank.
This led to the error message not being printed in certain cases. The
beeswax server had code to comprimise for this setback, which is now
removed.
Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
---
M be/src/common/status.cc
M be/src/common/status.h
M be/src/service/impala-beeswax-server.cc
M be/src/util/error-util.h
4 files changed, 30 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/6627/4
--
To view, visit http://gerrit.cloudera.org:8080/6627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 3:
(5 comments)
Running a private test to make sure none of the tests break due to specific expectations of Status behavior:
http://sandbox.jenkins.cloudera.com/view/Impala/view/Private-Utility/job/impala-private-build-and-test/5465/
http://gerrit.cloudera.org:8080/#/c/6627/3/be/src/common/status.cc
File be/src/common/status.cc:
PS3, Line 210: new ErrorMsg();
> not really relevant to your change, but I wonder if there's any reason why
That's true. But I'd rather not make the change in this patch, since there are some micro-optimizations done, regarding the 'delete' operator, etc. which will take some time to go through.
https://github.com/apache/incubator-impala/blob/master/be/src/common/status.h#L170
https://github.com/apache/incubator-impala/blob/master/be/src/common/status.h#L254
We could probably do it as later clean up if necessary.
PS3, Line 216: std::for_each(status.error_msgs.begin() + 1, status.error_msgs.end(),
: [&](string const& detail) { msg_->AddDetail(detail); });
> fancy
Ha. Read that this in many cases is better in perf than a regular for loop with iterators, not that it would matter that much here.
http://gerrit.cloudera.org:8080/#/c/6627/3/be/src/service/impala-beeswax-server.cc
File be/src/service/impala-beeswax-server.cc:
PS3, Line 288:
:
:
:
> the consistency issue aside, why remove this? is it because the information
Yes, because of duplication.
I can run a private test with the latest patch to see if all the tests pass to make sure.
PS3, Line 293: if (!exec_state->query_status().ok()) error_log_ss << "\n\n";
> I'm not sure if this still makes sense, if the query status is !ok you'll e
Oops, forgot to remove that. Done.
http://gerrit.cloudera.org:8080/#/c/6627/3/be/src/util/error-util.h
File be/src/util/error-util.h:
PS3, Line 116: string
> const reference
Done
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 4: Code-Review+1
lgtm pending test run passes
thanks!
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 5:
> > > > > (1 comment)
> > >
> > > > We have an error_log, which is different from error messages,
> > > which
> > > > is different from error details. There is a lot of overlap
> > > between
> > > > these which results in quite some confusion.
> > > >
> > > > The error_log is populated when we call LogError(). The error
> > > > message is the error associated with a status error. The
> error
> > > > details are extra error messages that we attach to a status.
> > > >
> > >
> > > Note that the error_log is really more like a warning log. i.e.
> > > these "errors" don't necessarily cause query execution to
> abort.
> > >
> > That's not always the case. In some places we add actual errors
> to
> > the error_log like here:
> > https://github.com/apache/incubator-impala/blob/master/be/src/runtime/plan-fragment-executor.cc#L343
> >
>
> Right, but we should always bubble up those kinds of errors as
> well. The point is that the converse is not true -- i.e. an error
> does not necessarily show up in the error log.
Yes, that's true.
>
> > > > Cumulatively, all tests together end up relying on all 3 of
> > these
> > > > being returned. Before this patch, the error_log was never
> > being
> > > > printed, but due to the overlap with the error message from
> the
> > > > status, all the tests passed. Now, that we print the
> error_log
> > > with
> > > > this patch, a lot of errors get printed twice, which looks
> > ugly.
> > > >
> > >
> > > Where does this patch cause the error log to be printed? And
> what
> > > do you mean by "printed"?
> > >
> > Printed meaning, returned to the client, so it prints on the
> > client's end (impala-shell)
>
> The main error status should be returned via GetOperationStatus(),
> not the error log.
>
> >
> > > Also, concerning the original issue, in what cases are error
> > > message being dropped?
> >
> > When we unpack messages from the TStatus object into a Status
> > object, we don't copy the original ErrorMsg::message_ back to the
> > new message_, instead we put everything in details_ causing the
> > message not to be printed (returned to the client), because
> > GetErrorLog() only looks at ErrorMsg::message_ and not details_.
>
> But GetOperationStatus() includes both, right?
>
> So, I guess what you're saying is that the error_log can drop some
> messages? But those messages should travel back to the coordinator
> as part of the error_log in TReportExecStatusParams. The one that
> gets returned as a TStatus should end up as *the* query status
> (i.e. GetOperationStatus()). (And only because Beeswax doesn't
> really have this, I think we overload get_log() to include the
> ultimate query_status()).
Yes, that's right. Also, since the query status and the error_log can have redundant information, I added some code to merge them so that it doesn't get printed twice.
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#2).
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
IMPALA-5198: Error messages are sometimes dropped before reaching client
The Status::ToThrift() function takes the ErrorMsg, and pushes both
the msg() and details() into the TStatus::error_msgs list.
However, when we unpack the TStatus object into a Status object, we
just copy all the TStatus::error_msgs to Status::ErrorMsg::details_
and leave Status::ErrorMsg::message_ blank.
This led to the error message not being printed in certain cases. The
beeswax server had code to comprimise for this setback, which is now
removed, and is similar in behavior to the HS2 server.
Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
---
M be/src/common/status.cc
M be/src/common/status.h
M be/src/service/impala-beeswax-server.cc
M be/src/util/error-util.h
4 files changed, 33 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/6627/2
--
To view, visit http://gerrit.cloudera.org:8080/6627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 7:
Thanks, this is much nicer.
Could you make sure we have a regression test for IMP-633 (there was no test with that commit), and if not, add one? We should make sure that that problem really was also solved by including the query_status explicitly in get_log().
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 4:
> > > > (1 comment)
> >
> > > We have an error_log, which is different from error messages,
> > which
> > > is different from error details. There is a lot of overlap
> > between
> > > these which results in quite some confusion.
> > >
> > > The error_log is populated when we call LogError(). The error
> > > message is the error associated with a status error. The error
> > > details are extra error messages that we attach to a status.
> > >
> >
> > Note that the error_log is really more like a warning log. i.e.
> > these "errors" don't necessarily cause query execution to abort.
> >
> That's not always the case. In some places we add actual errors to
> the error_log like here:
> https://github.com/apache/incubator-impala/blob/master/be/src/runtime/plan-fragment-executor.cc#L343
>
Right, but we should always bubble up those kinds of errors as well. The point is that the converse is not true -- i.e. an error does not necessarily show up in the error log.
> > > Cumulatively, all tests together end up relying on all 3 of
> these
> > > being returned. Before this patch, the error_log was never
> being
> > > printed, but due to the overlap with the error message from the
> > > status, all the tests passed. Now, that we print the error_log
> > with
> > > this patch, a lot of errors get printed twice, which looks
> ugly.
> > >
> >
> > Where does this patch cause the error log to be printed? And what
> > do you mean by "printed"?
> >
> Printed meaning, returned to the client, so it prints on the
> client's end (impala-shell)
The main error status should be returned via GetOperationStatus(), not the error log.
>
> > Also, concerning the original issue, in what cases are error
> > message being dropped?
>
> When we unpack messages from the TStatus object into a Status
> object, we don't copy the original ErrorMsg::message_ back to the
> new message_, instead we put everything in details_ causing the
> message not to be printed (returned to the client), because
> GetErrorLog() only looks at ErrorMsg::message_ and not details_.
But GetOperationStatus() includes both, right?
So, I guess what you're saying is that the error_log can drop some messages? But those messages should travel back to the coordinator as part of the error_log in TReportExecStatusParams. The one that gets returned as a TStatus should end up as *the* query status (i.e. GetOperationStatus()). (And only because Beeswax doesn't really have this, I think we overload get_log() to include the ultimate query_status()).
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5198: test exchange delays custom cluster test failed on s3 tests
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5198: test_exchange_delays custom cluster test failed on s3 tests
......................................................................
Patch Set 1:
(3 comments)
This is a nice catch
http://gerrit.cloudera.org:8080/#/c/6627/1//COMMIT_MSG
Commit Message:
Line 7: IMPALA-5198: test_exchange_delays custom cluster test failed on s3 tests
Maybe describe the symptoms/bug more generally, so it's easier to understand the impact. E.g. "Error message sometimes doesn't is dropped in HS2".
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 printing progress to the error log.
http://gerrit.cloudera.org:8080/#/c/6627/1/be/src/util/error-util.cc
File be/src/util/error-util.cc:
Line 123: if (detail.size() > 0) {
It seems like conceptually the problem is the Status(TStatus) constructor should do the inverse of Status::ToThrift() but doesn't actually. This fix solves the problem but now the packing of msg and details into a vector happens at a different level to the unpacking. I think this code would be easier to understand if it was restructured slightly so that the packing and unpacking happened at the same level.
Maybe we should just change ErrorMsg() to store a vector internally? Or we could add a method to ErrorMsg() to return a vector with all of the messages, which would be the inverse of the constructor?
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 9: Verified+1
--
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 2:
(2 comments)
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) {
> It's up for debate whether we want to add this or not. I added it to be con
That's interesting, didn't realise that.
My feeling is that it's best not to change this in this patchset. I'm not totally sure what the story is with HS2 - the thrift file says that GetLog() was removed in a Hive version.
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,
> I spent some time looking at the different places that TStatus is used, and
Looks good to me
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6627
to look at the new patch set (#5).
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
IMPALA-5198: Error messages are sometimes dropped before reaching client
The Status::ToThrift() function takes the ErrorMsg, and pushes both
the msg() and details() into the TStatus::error_msgs list.
However, when we unpack the TStatus object into a Status object, we
just copy all the TStatus::error_msgs to Status::ErrorMsg::details_
and leave Status::ErrorMsg::message_ blank.
This led to the error message not being printed in certain cases. The
beeswax server had code to comprimise for this setback, which is now
removed.
Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
---
M be/src/common/status.cc
M be/src/common/status.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/service/impala-beeswax-server.cc
M be/src/util/error-util.cc
M be/src/util/error-util.h
7 files changed, 55 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/6627/5
--
To view, visit http://gerrit.cloudera.org:8080/6627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 4:
> > (1 comment)
> We have an error_log, which is different from error messages, which
> is different from error details. There is a lot of overlap between
> these which results in quite some confusion.
>
> The error_log is populated when we call LogError(). The error
> message is the error associated with a status error. The error
> details are extra error messages that we attach to a status.
>
Note that the error_log is really more like a warning log. i.e. these "errors" don't necessarily cause query execution to abort.
> Cumulatively, all tests together end up relying on all 3 of these
> being returned. Before this patch, the error_log was never being
> printed, but due to the overlap with the error message from the
> status, all the tests passed. Now, that we print the error_log with
> this patch, a lot of errors get printed twice, which looks ugly.
>
Where does this patch cause the error log to be printed? And what do you mean by "printed"?
Also, concerning the original issue, in what cases are error message being dropped?
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 8: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6627/8/tests/data_errors/test_data_errors.py
File tests/data_errors/test_data_errors.py:
PS8, Line 58: ret
did you mean to use that?
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 4:
> > > (1 comment)
>
> > We have an error_log, which is different from error messages,
> which
> > is different from error details. There is a lot of overlap
> between
> > these which results in quite some confusion.
> >
> > The error_log is populated when we call LogError(). The error
> > message is the error associated with a status error. The error
> > details are extra error messages that we attach to a status.
> >
>
> Note that the error_log is really more like a warning log. i.e.
> these "errors" don't necessarily cause query execution to abort.
>
That's not always the case. In some places we add actual errors to the error_log like here:
https://github.com/apache/incubator-impala/blob/master/be/src/runtime/plan-fragment-executor.cc#L343
> > Cumulatively, all tests together end up relying on all 3 of these
> > being returned. Before this patch, the error_log was never being
> > printed, but due to the overlap with the error message from the
> > status, all the tests passed. Now, that we print the error_log
> with
> > this patch, a lot of errors get printed twice, which looks ugly.
> >
>
> Where does this patch cause the error log to be printed? And what
> do you mean by "printed"?
>
Printed meaning, returned to the client, so it prints on the client's end (impala-shell)
> Also, concerning the original issue, in what cases are error
> message being dropped?
When we unpack messages from the TStatus object into a Status object, we don't copy the original ErrorMsg::message_ back to the new message_, instead we put everything in details_ causing the message not to be printed (returned to the client), because GetErrorLog() only looks at ErrorMsg::message_ and not details_.
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#3).
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
IMPALA-5198: Error messages are sometimes dropped before reaching client
The Status::ToThrift() function takes the ErrorMsg, and pushes both
the msg() and details() into the TStatus::error_msgs list.
However, when we unpack the TStatus object into a Status object, we
just copy all the TStatus::error_msgs to Status::ErrorMsg::details_
and leave Status::ErrorMsg::message_ blank.
This led to the error message not being printed in certain cases. The
beeswax server had code to comprimise for this setback, which is now
removed.
Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
---
M be/src/common/status.cc
M be/src/common/status.h
M be/src/service/impala-beeswax-server.cc
M be/src/util/error-util.h
4 files changed, 30 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/6627/3
--
To view, visit http://gerrit.cloudera.org:8080/6627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 6:
> Uploaded patch set 6.
Made the changes as discussed above and ran an exhaustive build. All the tests passed save for one which was a manifestation of an unrelated issue (IMPALA-5080):
http://sandbox.jenkins.cloudera.com/job/impala-private-build-and-test/5488/
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 4:
> (1 comment)
I ran the tests and there were a few failures. I looked into the reasons and it seems like our mechanism for passing around error messages is even more complicated than I initially thought.
We have an error_log, which is different from error messages, which is different from error details. There is a lot of overlap between these which results in quite some confusion.
The error_log is populated when we call LogError(). The error message is the error associated with a status error. The error details are extra error messages that we attach to a status.
Cumulatively, all tests together end up relying on all 3 of these being returned. Before this patch, the error_log was never being printed, but due to the overlap with the error message from the status, all the tests passed. Now, that we print the error_log with this patch, a lot of errors get printed twice, which looks ugly.
I've updated the patch to take care of this. I'm running another private test to make sure all the tests pass before I put the next patchset out.
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6627/5/be/src/service/impala-beeswax-server.cc
File be/src/service/impala-beeswax-server.cc:
Line 301: if (!details.empty()) error_log_ss << "\n" << details;
Offline we discussed that we're going to try to remove the LogError() call from PlanFragmentExecutor::Exec() since that's where the error message redundancy comes from.
And we believe that LogError() (introduced at 89875939) is not necessary (after change 91db96d9 that added the code to get_log() to include query_status) to deal with the original problem (of query status not showing up anywhere in beeswax, which doesn't have GetOperationStatus()). Nor was it sufficient when the error occurs on the coordinator side, which is why reporting query_status in get_log() is better.
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
IMPALA-5198: Error messages are sometimes dropped before reaching client
The Status::ToThrift() function takes the ErrorMsg, and pushes both
the msg() and details() into the TStatus::error_msgs list.
However, when we unpack the TStatus object into a Status object, we
just copy all the TStatus::error_msgs to Status::ErrorMsg::details_
and leave Status::ErrorMsg::message_ blank.
This led to the error message not being printed in certain cases which
is now fixed.
The PlanFragmentExecutor had some code to add query statuses to
the error_log (IMP-633), which is no longer necessary after a
future patch (IMPALA-762) explicitly returned the query status to
the client via get_log(), making the adding of the query statuses
to the error_log redundant. That code in the PFE has been removed
and a test has been added to make sure that the case it previously
tried to fix doesn't regress.
Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
Reviewed-on: http://gerrit.cloudera.org:8080/6627
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/common/status.cc
M be/src/common/status.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/util/error-util.h
M tests/data_errors/test_data_errors.py
5 files changed, 51 insertions(+), 20 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Sailesh Mukil: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/6627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 8:
> Uploaded patch set 8.
Added a test to make sure IMP-633 doesn't regress.
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/6627/3/be/src/common/status.cc
File be/src/common/status.cc:
PS3, Line 210: new ErrorMsg();
not really relevant to your change, but I wonder if there's any reason why we don't keep this in a unique_ptr so we don't have to explicitly delete it in the destructor.
PS3, Line 216: std::for_each(status.error_msgs.begin() + 1, status.error_msgs.end(),
: [&](string const& detail) { msg_->AddDetail(detail); });
fancy
http://gerrit.cloudera.org:8080/#/c/6627/3/be/src/service/impala-beeswax-server.cc
File be/src/service/impala-beeswax-server.cc:
PS3, Line 288:
:
:
:
the consistency issue aside, why remove this? is it because the information is duplicated in the GetErrorLog() output?
are we sure we don't have any tests that rely on the order in which this info is printed?
PS3, Line 293: if (!exec_state->query_status().ok()) error_log_ss << "\n\n";
I'm not sure if this still makes sense, if the query status is !ok you'll end up with a few extra newlines at the top of the returned string.
http://gerrit.cloudera.org:8080/#/c/6627/3/be/src/util/error-util.h
File be/src/util/error-util.h:
PS3, Line 116: string
const reference
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................
Patch Set 9:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/486/
--
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No