Michael Ho 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 14: Code-Review+1 (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 > I think it's better to rely on the enum ordering. Otherwise, the new boolea My understanding is that explicitly check the current state during state transition but don't rely on the ordering of the enum in any way. That said, it'd be great if the enum is ordered in the expected state transition order. May help to have a static DCHECK somewhere to confirm the ordering of the enum entries matches the expected state transition order if we don't have one already. http://gerrit.cloudera.org:8080/#/c/10813/13/be/src/runtime/fragment-instance-state.cc@522 PS13, Line 522: "First batch produced", // FIRST_BATCH_PRODUCED : "Producing Data", // PRODUCING_DATA This needs to be changed. I think this is dependent on the enum ordering. 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 enum, can you please add a comment and ideally a DCHECK for it ? -- 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: 14 Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Mon, 06 Aug 2018 19:55:44 +0000 Gerrit-HasComments: Yes
