Dan Hecht 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 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/8758/2/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/8758/2/be/src/runtime/coordinator-backend-state.cc@512 PS2, Line 512: value->AddMember("done", done_, document->GetAllocator()); : : Value state_val( : FragmentInstanceState::GetFInstanceStateDescription(current_state_).c_str(), : document->GetAllocator()); : value->AddMember("current_state", state_val, document->GetAllocator()); is 'done' completely redundant with 'current_state', or can it not always be implied? i guess for the error path the state can be anything when done==true? The error case is probably worth testing as well (make sure things look sane when a FIS ends in error). http://gerrit.cloudera.org:8080/#/c/8758/2/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/8758/2/be/src/runtime/fragment-instance-state.cc@296 PS2, Line 296: bool first_batch_received = false; : bool first_batch_sent = false; : do { : Status status; : row_batch_->Reset(); : { : if (!first_batch_received) { : UpdateCurrentState(TFInstanceState::WAITING_FOR_FIRST_BATCH); : } : SCOPED_TIMER(plan_exec_timer); : RETURN_IF_ERROR( : exec_tree_->GetNext(runtime_state_, row_batch_.get(), &exec_tree_complete)); : if (!first_batch_received) { : first_batch_received = true; : UpdateCurrentState(TFInstanceState::FIRST_BATCH_RECEIVED); : } else if (current_state_.load() != TFInstanceState::RECEIVING_DATA) { : UpdateCurrentState(TFInstanceState::RECEIVING_DATA); : } : } : 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())); : if (!first_batch_sent) { : first_batch_sent = true; : UpdateCurrentState(TFInstanceState::FIRST_BATCH_SENT); : } : } while (!exec_tree_complete); : : UpdateCurrentState(TFInstanceState::LAST_BATCH_SENT); I think all the state transition logic would be easier to follow if we explicitly made it a state machine. The stuff in thrift would be the states, but this file could define "transitions" and the rules: Transition(state) => new_state i.e. UpdateCurrentState() would take a transition and use that and the current state to determine the new state. That way, all of this complicated state machine logic is encapsulated in one place and doesn't complicate the core execution code. I think that will make it easier to comment on specifics of the states/transitions so I'll hold off on looking at that too closely yet. http://gerrit.cloudera.org:8080/#/c/8758/2/be/src/runtime/fragment-instance-state.cc@443 PS2, Line 443: default: : // Do nothing this would be easier to understand if you explicitly list out all cases. though this code will change with the above suggestion. http://gerrit.cloudera.org:8080/#/c/8758/2/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/8758/2/be/src/util/runtime-profile.cc@310 PS2, Line 310: } what's the summary for why this change is needed? http://gerrit.cloudera.org:8080/#/c/8758/2/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/8758/2/common/thrift/ImpalaInternalService.thrift@639 PS2, Line 639: 5: optional TFInstanceState current_state given this interacts with IMPALA-2990 work, we may want to split this change out into two: the first could add the instance timeline, and then second could expose it to the webpage. But let's defer that decision for now and leave it together as we iterate on this change. -- 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: 2 Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Comment-Date: Tue, 05 Dec 2017 19:31:22 +0000 Gerrit-HasComments: Yes
