impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2905: Handle coordinator fragment lifecycle like all others
Date Wed, 05 Oct 2016 00:45:04 GMT
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 <henry@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message