Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-2905: Handle coordinator fragment lifecycle like all 
others
......................................................................


Patch Set 6:

(24 comments)

http://gerrit.cloudera.org:8080/#/c/4402/6/be/src/exec/push-pull-sink.h
File be/src/exec/push-pull-sink.h:

Line 29: /// Sink which queues incoming batches from fragment, allowing 
consumers to pull from the
QueueSink then? i couldn't quite figure out what PushPullSink was.


Line 30: /// queue at their own pace by calling GetNext().
what's the size of the queue?


Line 66:   virtual Status GetNext(RuntimeState* state, RowBatch** row_batch);
why virtual?


Line 77:   /// copying overhead; since RowBatch is not movable it's wrapped in 
a unique_ptr.
re should be movable: i don't particularly like this comment, since it seems to 
imply that 'moving is always okay' (which i disagree with, and i wouldn't want 
RowBatches to be moved in and out of this queue). happy to talk about it 
offline.


http://gerrit.cloudera.org:8080/#/c/4402/6/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 484:     // To prepare the expressions we need a local RuntimeState and 
DescriptoTbl.
spelling


Line 486:     RETURN_IF_ERROR(DescriptorTbl::Create(obj_pool(), desc_tbl_, 
&descriptor_table));
leave todo to get that out of the qs. this is expensive to create.


Line 505:   RETURN_IF_ERROR(StartRemoteFragments(&schedule));
rename to StartFragments


Line 510:     // Coordinator fragment instance has same ID as query.
that's actually not true for the mt case.


Line 1471:   // Make sure that InitExecProfile() has been called.
probably best to fix initialization as part of this change (it should precede 
everything else)


Line 1637:   // May not be called for the coordinator fragment, which has no 
averaged profile.
that shouldn't be true anymore


http://gerrit.cloudera.org:8080/#/c/4402/6/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

Line 266:   /// them to the client in GetNext().
why do you need this and not just the queue sink?


Line 277:   MemTracker* output_expr_tracker_ = nullptr;
are we tracking this right now?


Line 381:   /// dependencies.
i don't understand why that's needed, let's talk in person.


Line 431:   /// TODO: Remove when output exprs evaluated by PushPullSink.
i don't understand why a sink would want to deal with exprs, let's talk in 
person.


http://gerrit.cloudera.org:8080/#/c/4402/6/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

Line 286:   if (sink_->GetName() == PushPullSink::NAME) {
use fragment.output_sink.type instead


Line 359:   if (status.ok()) status = DriveSink();
it looks like we should break up Open() and move the row-producing logic into a 
new function (then the semantics of opened_promise_ would be easy to describe)


http://gerrit.cloudera.org:8080/#/c/4402/6/be/src/runtime/plan-fragment-executor.h
File be/src/runtime/plan-fragment-executor.h:

Line 49: /// PlanFragmentExecutor handles all aspects of the execution of a 
single plan fragment,
extend class description with new semantics.


Line 166:   bool done_;
i don't think 'done' quite captures that (we use that word too much). opened?


http://gerrit.cloudera.org:8080/#/c/4402/6/be/src/service/fragment-mgr.cc
File be/src/service/fragment-mgr.cc:

Line 60
don't all fragments now have a sink?


Line 46:   lock_guard<SpinLock> l(fragment_exec_state_map_lock_);
why does this need to happen up here?


http://gerrit.cloudera.org:8080/#/c/4402/6/be/src/service/impala-internal-service.h
File be/src/service/impala-internal-service.h:

Line 36:       FragmentMgr* fragment_mgr)
why even have that param if it's a singleton


http://gerrit.cloudera.org:8080/#/c/4402/6/be/src/util/blocking-queue.h
File be/src/util/blocking-queue.h:

Line 35: /// Queue entries must be movable.
why is this needed for these changes?


http://gerrit.cloudera.org:8080/#/c/4402/6/common/thrift/DataSinks.thrift
File common/thrift/DataSinks.thrift:

Line 90: struct TPushPullSink {
remove


Line 108:   5: optional TPushPullSink push_pull_sink
remove, we already have 'type'


-- 
To view, visit http://gerrit.cloudera.org:8080/4402
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb0064ec2f085fa3a5598ea80894fb489a01e4df
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to