impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6190/6246: Add instances tab and event sequence
Date Sat, 09 Dec 2017 02:48:13 GMT
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 <lv@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Comment-Date: Sat, 09 Dec 2017 02:48:13 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message