Dan Hecht has posted comments on this change. Change subject: IMPALA-1633: Impala server GetOperationStatus() should return detail sql error code and message when query fails ......................................................................
Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/3094/2/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: Line 939: QueryState::type query_state = entry->second->query_state(); this might read a little easier if we take a pointer to the QueryExecState rather than the query_state field, so then we can use it below too: QueryExecState *exec_state = entry->second.get(); ... QueryStateToTOperationState(exec_state->query-state()); if (!exec_state->query_status().ok()) { ... } Also, does HS2 say anything about the relationship between operationState and errorMessage i.e. is errorMessage allowed to be set only for certain operationStates (like ERROR_STATE)? Does impala have any guarantees on when query_status is !ok() what the possible values of query_state? http://gerrit.cloudera.org:8080/#/c/3094/2/tests/hs2/test_hs2.py File tests/hs2/test_hs2.py: Line 156: TestHS2.check_response(get_operation_status_resp) could we use your new get_operation_status() in various other places in this file as well such as here? Line 180: get_operation_status_resp.sqlState is not "" would another interesting test case be to do: ExecuteStatement() Fetch() or don't Fetch() GetOperationStatus() where the error occurs asynchronously (in our current implementation for the test case you have, the error occurs synchronously with ExecuteStatement()). We should have some tables somewhere that should induce an error during query execution. That would more closely mimic IMPALA-3298. -- 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: 2 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-HasComments: Yes
