Henry Robinson has posted comments on this change.

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

Patch Set 7:


This patch passes EE tests.

I haven't completely addressed the issue of exec profiles in the coordinator - 
I intend to rebase on the MT coordinator patch which reworks that path.

The major change is that the new sink hands off a single batch from the sender 
to the consumer at a time. Queuing and resource transfer are orthogonal 
concerns; the main benefit of this patch is the simplification of QES, 
coordinator and PFE paths.

File be/src/exec/push-pull-sink.cc:

Line 31
> i think if you have the sink build a queue of QueryResultSet* (and change Q
To make progress, I changed this so that the QES passes in a QueryResultSet via 
the coordinator, which is filled by GetNext().

We can look into constructing the QRS in this sink, and the memory and perf 
effects of queuing, in a separate patch.

Line 54
> Will this thread be unblocked when the query is cancelled? I imagine a scen
Implementation changed, not applicable.

Line 58
> The row batches still sitting in the queue are Reset() implicitly when this
Implementation changed, not applicable.

File be/src/exec/push-pull-sink.h:

Line 29
> QueueSink then? i couldn't quite figure out what PushPullSink was.
Renamed to PlanRootSink.

Line 30
> what's the size of the queue?

Line 66
> why virtual?

Line 77
> re should be movable: i don't particularly like this comment, since it seem
Moot for now, since the queue is removed.

File be/src/runtime/coordinator.cc:

Line 356:     num_remaining_fragment_instances_(0),
> Set in Exec()

Line 471: 
> Can we get rid of rm_reserved_limit_ in MemTracker?

Line 484:   if (has_coordinator_fragment) {
> spelling

Line 486:     root_fragment_instance_ =
> leave todo to get that out of the qs. this is expensive to create.

Line 490:     executor_->WaitForPrepare();
> const TPlanNode& ?

Line 491: 
> my_row_desc -> root_row_desc

Line 510:     int num_hosts, int start_fragment_instance_idx) {
> that's actually not true for the mt case.
As discussed, we should make it true :)

Line 1471:   }
> probably best to fix initialization as part of this change (it should prece
Will defer to after rebasing on top of your changes which improve the profile 
creation path.

Line 1472: 
> Is this always set, even in weird error scenarios?
Same - going to defer until rebase on MT coord patch.

Line 1637:   }
> that shouldn't be true anymore
Will defer.

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?
For row_desc() and runtime_state().

File be/src/runtime/plan-fragment-executor.cc:

Line 339:   return Status::OK();
> Could another thread wait for opened_promise_ indefinitely if this returns 
Yes, fixed by moving all the error-prone logic into OpenInternal()

Line 359:   while (report_thread_active_) {
> it looks like we should break up Open() and move the row-producing logic in
Good idea. Added Exec() which produces rows. Only the FES calls this API, which 
now runs:

  if (Prepare().ok()) {

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.
What are the new semantics? The GetNext() path is removed, but there's nothing 
new in its place as far as mechanisms in PFE go.

File be/src/service/fragment-mgr.cc:

Line 60
> don't all fragments now have a sink?
Yes, but I think this is the wrong place to check it. The PFE already checks in 
Prepare(), so I think this check was mostly to be sure that we handed tried to 
remotely execute the coordinator fragment.

Line 46:   // Preparing and opening the fragment creates a thread and consumes 
a non-trivial
> why does this need to happen up here?

File be/src/service/impala-internal-service.h:

Line 36:     : impala_server_(impala_server) {
> why even have that param if it's a singleton

File be/src/util/blocking-queue.h:

Line 35: 
> why is this needed for these changes?
It's not any more now that there's no queue in the new sink.

File common/thrift/DataSinks.thrift:

Line 90: struct TTableSink {
> remove

Line 108
> remove, we already have 'type'

File fe/src/main/java/com/cloudera/impala/planner/PushPullSink.java:

Line 29
> Imo it would be very nice to eventually have this be a ResultSetSink. Apart
Agreed. My latest patch moves expr evaluation into the sink. However, let's put 
the exprs in the explain plan in a separate patch - it's a bit more involved to 
move the result exprs around than it looks at first glance.

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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to