Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8758 )
Change subject: IMPALA-6190/6246: Add instances tab and event sequence ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/8758/3/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/8758/3/be/src/runtime/coordinator-backend-state.h@129 PS3, Line 129: InstancesToJson Is InstanceStatsToJson() a more appropriate name ? http://gerrit.cloudera.org:8080/#/c/8758/3/be/src/runtime/fragment-instance-state.h File be/src/runtime/fragment-instance-state.h: http://gerrit.cloudera.org:8080/#/c/8758/3/be/src/runtime/fragment-instance-state.h@172 PS3, Line 172: PREPARE_START, : CODEGEN_START, : CODEGEN_END, : OPEN_START, : WAITING_FOR_BATCH, : BATCH_RECEIVED, : BATCH_SENT, : END_OF_STREAM, : EXEC_END It'd be nice to add comments to indicate what each of this state means. http://gerrit.cloudera.org:8080/#/c/8758/3/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/8758/3/be/src/runtime/fragment-instance-state.cc@241 PS3, Line 241: UpdateState(StateEvent::CODEGEN_END); Doesn't this overlap with the codegen time shown in the RuntimeProfile ? Also, doesn't moving to the next state imply the end of codegen ? http://gerrit.cloudera.org:8080/#/c/8758/3/be/src/runtime/fragment-instance-state.cc@302 PS3, Line 302: UpdateState(StateEvent::WAITING_FOR_BATCH); : SCOPED_TIMER(plan_exec_timer); : RETURN_IF_ERROR( : exec_tree_->GetNext(runtime_state_, row_batch_.get(), &exec_tree_complete)); : UpdateState(StateEvent::BATCH_RECEIVED); : } : if (VLOG_ROW_IS_ON) row_batch_->VLogRows("FragmentInstanceState::ExecInternal()"); : COUNTER_ADD(rows_produced_counter_, row_batch_->num_rows()); : RETURN_IF_ERROR(sink_->Send(runtime_state_, row_batch_.get())); : UpdateState(StateEvent::BATCH_SENT); In terms of observability, how much benefit is there for updating the state once for every row batch produced vs just updating the state once before entering the loop to indicate that we are producing row ? I guess the finer granularity is helpful if a fragment instance is stuck but then we can also infer that from various timers in the runtime profile too. On the other hand, that's kind of the point of this patch to make it easier to find out at which state of execution is a fragment instance stuck. I am a bit worried about the cost of updating the state as UpdateState() looks a bit heavy weight too. I wonder if just recording the state of a fragment instance: PREPARE, OPEN, EXEC, and EOS is sufficient for initial pass or is it not enough details ? http://gerrit.cloudera.org:8080/#/c/8758/3/be/src/service/impala-http-handler.h File be/src/service/impala-http-handler.h: http://gerrit.cloudera.org:8080/#/c/8758/3/be/src/service/impala-http-handler.h@99 PS3, Line 99: backend states fragment instances ? -- To view, visit http://gerrit.cloudera.org:8080/8758 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I626456b6afa9101eeeeffd5cda10c4096d63d7f9 Gerrit-Change-Number: 8758 Gerrit-PatchSet: 3 Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Comment-Date: Sat, 09 Dec 2017 02:48:13 +0000 Gerrit-HasComments: Yes
