Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11615 )
Change subject: IMPALA-4063: Merge report of query fragment instances per executor ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.h@185 PS2, Line 185: status reports periodically : /// to > nit: status reports periodically to the Done http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.h@428 PS2, Line 428: Should only be called when : /// the current state is a non-terminal state. T > A little confusing - its required to currently be in a non-terminal state w Done http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.cc@227 PS2, Line 227: { > any reason to do this here, when its only used within the lock below? Done http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.cc@243 PS2, Line 243: backend_exec_state_ = cur_state == BackendExecState::PREPARING ? : BackendExecState::EXECUTING : BackendExecState::FINISHED; > This feels a little bit weird - would it work to make this more explicit by The state transition logic is determined by 'overall_status_'. So, making UpdateBackendExecState() take a param of the next state means moving the logic encoded in the if-stmt above elsewhere and UpdateBackendExecState() simply becomes a setter of backend_exec_state_ with some DCHECKs(). I can see how it may be slightly clearer for the transition from PREPARING state to the next legal state but it may make the code slightly more hairy at the multiple call sites. Will it be clearer if we update the DCHECK at line 231 to make it more explicit which valid states we could be at so it's easier to reason about the state transition logic ? http://gerrit.cloudera.org:8080/#/c/11615/2/common/protobuf/control_service.proto File common/protobuf/control_service.proto: http://gerrit.cloudera.org:8080/#/c/11615/2/common/protobuf/control_service.proto@150 PS2, Line 150: // The fragment instance id of the first failed fragment instance. > Maybe worth making it more explicit that the reason we chose the 'first' he It's not set for "general" error. Comments clarified to update its relationship with "overall_status". -- To view, visit http://gerrit.cloudera.org:8080/11615 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa Gerrit-Change-Number: 11615 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 10 Oct 2018 21:57:22 +0000 Gerrit-HasComments: Yes
