Sahil Takiar 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 3: (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: illegal state tran > the (case->if) statement in UpdateNonErrorOperationState would ensure that Good point. Updated the commit message. wrt UpdateNonErrorOperationState, it seems the current behavior is to ignore state transitions that are invalid, I think it would be cleaner to enforce the transitions using DCHECKs and fix any attempts to perform an illegal state transition. this can help surface bugs in the code if the an illegal state transition is ever attempted. 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 guarant changed it so that if there is a async_exec_thread_ thread, it sets the state to PENDING before the async_exec_thread_ is created. this way when the Exec RPC returns, the state should always be PENDING. http://gerrit.cloudera.org:8080/#/c/14744/2/be/src/service/client-request-state.cc@780 PS2, Line 780: result_metadata_ = metadata_op_result.schema > I think with a metadata op it is expected to transition directly to FINISHE Yeah, I made this change because thats what all the other operations do, even the ones that don't run asynchronously. So I think it makes sense to keep this consistent with the rest of the operations. Plus, it makes the state machine easier to reason about if everything transitions from INITIALIZED -> [PENDING] -> RUNNING -> FINISHED. http://gerrit.cloudera.org:8080/#/c/14744/2/be/src/service/client-request-state.cc@929 PS2, Line 929: $0 -> $1"; > nit: how about: Done -- 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: 3 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: Fri, 22 Nov 2019 03:37:43 +0000 Gerrit-HasComments: Yes
