Thomas Marshall 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 2: (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 report periodically : /// the nit: status reports periodically to the http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.h@428 PS2, Line 428: A state transition happens : /// if the current state is a non-terminal state A little confusing - its required to currently be in a non-terminal state when this is called. 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: BackendExecState old_state = backend_exec_state_; any reason to do this here, when its only used within the lock below? http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.cc@243 PS2, Line 243: backend_exec_state_ = old_state == BackendExecState::PREPARING ? : BackendExecState::EXECUTING : BackendExecState::FINISHED; This feels a little bit weird - would it work to make this more explicit by having UpdateBackendExecState take a param of the state to transition to, and then DCHECK-ing that the param is the next state in the lifecycle? 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' here is that it corresponds to the value of 'overall_status' Also, what's the expected value here if 'overall_status' is for a non-fragment-specific "general" error? -- 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: 2 Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@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: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 10 Oct 2018 21:04:19 +0000 Gerrit-HasComments: Yes
