Henry Robinson has posted comments on this change. Change subject: IMPALA-2905: Handle coordinator fragment lifecycle like all others ......................................................................
Patch Set 7: (28 comments) 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. http://gerrit.cloudera.org:8080/#/c/4402/6/be/src/exec/push-pull-sink.cc 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. 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 > QueueSink then? i couldn't quite figure out what PushPullSink was. Renamed to PlanRootSink. Line 30 > what's the size of the queue? Done Line 66 > why virtual? Done Line 77 > re should be movable: i don't particularly like this comment, since it seem Moot for now, since the queue is removed. http://gerrit.cloudera.org:8080/#/c/4402/6/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: Line 356: num_remaining_fragment_instances_(0), > Set in Exec() Done Line 471: > Can we get rid of rm_reserved_limit_ in MemTracker? Done Line 484: if (has_coordinator_fragment) { > spelling Done Line 486: root_fragment_instance_ = > leave todo to get that out of the qs. this is expensive to create. Fixed. Line 490: executor_->WaitForPrepare(); > const TPlanNode& ? Done Line 491: > my_row_desc -> root_row_desc Done 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. 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? For row_desc() and runtime_state(). http://gerrit.cloudera.org:8080/#/c/4402/6/be/src/runtime/plan-fragment-executor.cc 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()) { Open(); Exec(); } Close(); 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. 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. 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? 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? Reverted. 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: : impala_server_(impala_server) { > why even have that param if it's a singleton Done http://gerrit.cloudera.org:8080/#/c/4402/6/be/src/util/blocking-queue.h 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. http://gerrit.cloudera.org:8080/#/c/4402/6/common/thrift/DataSinks.thrift File common/thrift/DataSinks.thrift: Line 90: struct TTableSink { > remove Done Line 108 > remove, we already have 'type' Done http://gerrit.cloudera.org:8080/#/c/4402/6/fe/src/main/java/com/cloudera/impala/planner/PushPullSink.java 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
