impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-1633: GetOperationStatus should set errorMessage and sqlState
Date Wed, 25 May 2016 21:42:13 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-1633: GetOperationStatus should set errorMessage and sqlState
......................................................................


Patch Set 5:

(3 comments)

Looking good, thanks! Just a few more small things, almost there...

http://gerrit.cloudera.org:8080/#/c/3094/5/be/src/service/query-exec-state.cc
File be/src/service/query-exec-state.cc:

Line 674: UpdateQueryState
I think it might be worth having this DCHECK if query_state is an exception state because
it would allow state_ to be error without an error status being set. The header comment already
indicates this is only for non-error states. I'd also suggest changing the name to indicate
that as well, but I can't think of a good name (UpdateNonErrorQueryState()? Blegh).


http://gerrit.cloudera.org:8080/#/c/3094/5/tests/hs2/hs2_test_suite.py
File tests/hs2/hs2_test_suite.py:

Line 213: HS2TestSuite.check_response(get_operation_status_resp, expected_status_code)
I suspect this may be more confusing than it's worth. Imagine someone adds a test which uses
this, but maybe doesn't know what the RPC status will be (e.g. maybe calls this repeatedly
until an error occurs). It's not clear this fn would perform this check either, even in the
code below (l224) we duplicate this check :) It's a common pattern in our hs2 test code, so
I don't think it's a big deal. Can you just have the callers do this check and remove the
optional parameter?


http://gerrit.cloudera.org:8080/#/c/3094/5/tests/hs2/test_hs2.py
File tests/hs2/test_hs2.py:

Line 175: get_operation_status_resp.sqlState
do we know what this will be?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icb792f88286779fcf2ce409828de818bc4e80bed
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message