Lars Volker 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) > Patch Set 3: > > (5 comments) Thank you for the review. Please see the new PS and my inline replies. 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 ? Done 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. Done 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 ? Al Yes, these overlap. I thought it'd still be nice to show it in the event sequence, too. Should I remove them? We do some more work below, including allocating the row batch and creating the reporting thread, so I thought it'd be good to see whether we're stuck / spending time in codegen vs there. 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 I think it's helpful to track "first batch received/sent" separately since it indicates the end of waiting for the producers. This allows us to see why a fragment may take longer than expected (blocked on receiver vs blocked on sender). We could probably skip the state transition to "Receiving data" but then the output will be harder to interpret. This picture shows an example of how it currently looks: https://git.io/vbEc4 The distinction between "Receiving" and "Waiting for First Batch" seems interesting to me. Once we're in TFInstanceState::RECEIVING_DATA, UpdateState() does a Load(), a switch, and 1-2 branches. No assignments and no Store() are necessary on the common path for batches after the first one. I added branch hints to those paths in the next PS. If you're concerned, I will do a perf run to make sure there are no regressions. We could also get rid of the Load() since current_state_ is not written anywhere else. That would introduce another member though. 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 ? Done -- 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: Mon, 11 Dec 2017 19:18:11 +0000 Gerrit-HasComments: Yes
