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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
