Thomas Tauber-Marshall 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: (8 comments) Updated to the reviews. I also removed QueryExecState::set_query_state as its not being used anywhere, and removing it makes it easier to trace through all of the code paths where query_state_ gets set, and I added some comments clarifying some of our behavior around query_state_/query_status_. I'm almost certain that query-exec-state.cc:846 (query_state_ = QueryState::EXCEPTION;) could be removed, as it should be impossible for query_state_ to not already be EXCEPTION if you reach this line, and doing so would further clarify our behavior, but its a riskier change so I left it for now (I did try changing it to a DCHECK and running all of the tests locally, and nothing failed) 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 Yes, errorMessage should only be set if operationState is ERROR_STATE. Currently, there are no code paths where query_state_ is set to EXCEPTION but query_status_ is not set to an error status. (this is easy to see in the code as there's only one line where query_status_ gets updated, and the line immediately before it sets query_state_ to EXCEPTION). However, these variables are supposed to be protected by a lock, and there is the potential for a race condition since we don't take the lock in GetOperationStatus. After talking with Matt, we think the right solution is to just take the query_exec_state lock in GetOperationStatus, since accesses to query_status_ and query_state_ really should be protected. The main concern here is that it could cause GetOperationStatus to block if the lock is already held, but we think this shouldn't be a problem in practice. http://gerrit.cloudera.org:8080/#/c/3094/2/tests/hs2/hs2_test_suite.py File tests/hs2/hs2_test_suite.py: Line 215: status > state Its a little unclear, since its GetOperationStatus vs. operationState, but I agree its probably better to say state here. Line 216: > extra spaces? We're inconsistent about this, eg. some of tests/common/impala_service.py is formatted this way, some of tests/common/impala_test_suite.py is formatted more in our c++ style. I'll at least make it consistent with the other functions in this file. Do we have a python style guide we use? Line 217: tatus > state Done Line 218: GetOperationStatus every interval seconds, returning the TGetOperationStatusResp""" > , or raises an assertion if it times out. Done Line 226: 'Did not reach expected operation status in time > can you include what it is and what it was expected to be? Done 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 thi Done Line 180: get_operation_status_resp.sqlState is not "" > would another interesting test case be to do: Adding a Fetch is tricky as Fetch will unregister a query if it sees that its in an error state, after which we can't call GetOperationStatus as it'll just return an invalid operation handle error. I don't think that the error here occurs synchronously with ExecuteStatement - the first time the state is retrieved after ExecuteStatement, its usually RUNNING_STATE. -- 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-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
