Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14744 )
Change subject: IMPALA-6894: Use an internal representation of query states in ClientRequestState ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/14744/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14744/2//COMMIT_MSG@27 PS2, Line 27: RUNNING to PENDING the (case->if) statement in UpdateNonErrorOperationState would ensure that it does not transition back to a previous state. This was added specifically because of the decision to transition to PENDING state in the Exec RPC thread. http://gerrit.cloudera.org:8080/#/c/14744/2/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/14744/2/be/src/service/client-request-state.cc@a278 PS2, Line 278: : : i think the reason we chose to do this here is because we wanted to guarantee that the state transitions to the next state (PENDING or RUNNING) when the client's Exec RPC returns. I am not sure if there are any tests that rely on this assumption (if there are then your change can make them flaky) http://gerrit.cloudera.org:8080/#/c/14744/2/be/src/service/client-request-state.cc@780 PS2, Line 780: UpdateNonErrorExecState(ExecState::RUNNING); I think with a metadata op it is expected to transition directly to FINISHED since there is actually nothing executing at this point. Also if we set this to running here, the caller of this ImpalaServer::ExecuteMetadataOp immediately transitions it to a terminal state (FINISHED or ERROR). Dont feel strongly about this either way, so feel free to ignore. http://gerrit.cloudera.org:8080/#/c/14744/2/be/src/service/client-request-state.cc@929 PS2, Line 929: Unexpected state nit: how about: "Illegal state transition: " << ExecStateToString(old_state) << " -> " << ExecStateToString(new_state) -- To view, visit http://gerrit.cloudera.org:8080/14744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1ce70bd2e964b309ebfc9d6ff6d900485db4d630 Gerrit-Change-Number: 14744 Gerrit-PatchSet: 2 Gerrit-Owner: Sahil Takiar <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Wed, 20 Nov 2019 22:00:38 +0000 Gerrit-HasComments: Yes
