Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10813 )
Change subject: IMPALA-7163: Implement a state machine for the QueryState class ...................................................................... Patch Set 15: (3 comments) http://gerrit.cloudera.org:8080/#/c/10813/13/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/10813/13/be/src/runtime/fragment-instance-state.cc@107 PS13, Line 107: er > My understanding is that explicitly check the current state during state tr I think the only way to have a reliable DCHECK is to have a boolean that denotes whether the instance was prepared or not. So, I went ahead and took your suggestion to add the 'is_prepared' local flag. So, in case someone changes the order of enums in the future, this DCHECK will catch it. http://gerrit.cloudera.org:8080/#/c/10813/13/be/src/runtime/fragment-instance-state.cc@522 PS13, Line 522: // Labels to send to the debug webpages to display the current state to the user. : static const string finstance_state_labels[] = { > This needs to be changed. I think this is dependent on the enum ordering. Good catch. Done. http://gerrit.cloudera.org:8080/#/c/10813/14/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/10813/14/common/thrift/ImpalaInternalService.thrift@626 PS14, Line 626: // the debug webpages. > Given the assumption in this patch about the ordering of the fields in this Done. Not adding the DCHECKs for every state though as that may be excessive. However, I've added a few DCHECKs at appropriate places in the code to ensure that some of the states are reached in order. -- To view, visit http://gerrit.cloudera.org:8080/10813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec5670a7db83ecae4656d7bb2ea372d3767ba7fe Gerrit-Change-Number: 10813 Gerrit-PatchSet: 15 Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Tue, 07 Aug 2018 00:57:27 +0000 Gerrit-HasComments: Yes
