Henry Robinson has posted comments on this change.

Change subject: IMPALA-4014: HEADERS ONLY: Introduce query-wide execution state.
......................................................................


Patch Set 1:

(17 comments)

http://gerrit.cloudera.org:8080/#/c/4418/1/be/src/runtime/fragment-instance-state.h
File be/src/runtime/fragment-instance-state.h:

PS1, Line 32: setting
            : /// that flag to 0 disables periodic reporting altogether.
Possibly a good time to retire that functionality which I don't think is ever 
used.


PS1, Line 43: TODO:
            : /// - move tear-down logic out of d'tor and into TearDown() 
function
Why can't we do that in this patch (given it's header only...)?


PS1, Line 56: QueryState* query_state() { return query_state_; }
When would we want to get a mutable query_state() from its FIS? That 
circumstance would suggest the caller violated the top-down pattern for getting 
at the FIS in the first place.


PS1, Line 69: /// Set the execution thread, taking ownership of the object.
            :   void set_exec_thread(Thread* exec_thread) { 
exec_thread_.reset(exec_thread); }
I do find this API weird, always have. Why doesn't this object start its own 
thread?


PS1, Line 116:   /// Returns true if this query has a limit and it has been 
reached.
             :   bool ReachedLimit();
FYI, this and GetNext() are likely to be removed with my patch for IMPALA-2905 
(subject to review). Might be worth anticipating that here.


http://gerrit.cloudera.org:8080/#/c/4418/1/be/src/runtime/query-exec-mgr.h
File be/src/runtime/query-exec-mgr.h:

PS1, Line 24: an entry point for
            : /// execution-related rpcs (ExecPlanFragment/CancelPlanFragment)
I don't think this is relevant - we shouldn't comment on what might call this 
class.


PS1, Line 33: Also creates a QueryState for this query, if none exists, and 
sets its refcount
            :   /// to 1. If a QueryState already exists for this query, 
increments the refcount.
Not clear from this who owns that reference, and who is therefore responsible 
for releasing it.


PS1, Line 43: /// Cancels a particular fragment instance.
            :   Status CancelFInstance(const TUniqueId& finstance_id);
Why is this here, and not accessible through 
GetQueryState(query_id)->Cancel(...) ?


PS1, Line 55: PublishFilter
Again, don't see that this needs to be in this class.


http://gerrit.cloudera.org:8080/#/c/4418/1/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:

PS1, Line 31: threads
too specific: clients of this class must obtain a reference.


PS1, Line 41: class Guard {
comment on usage pattern, which is going to be common. Particularly that 
callers must check if query_state() is not null, and if it is not null then it 
will live as long as the guard.

FWIW, I prefer ScopedRef as a name. The 'guard' in lock_guard<> means - I think 
- that the object guards a critical section by preventing concurrent access.


PS1, Line 44: (ExecEnv::GetQueryExecMgr()
shouldn't there be corresponding changes to ExecEnv then? Or are you leaving 
those out to make this self-contained?

Either way - prefer to do ExecEnv::GetInstance()->query_exec_mgr(). Under what 
circumstances would this ever be null?


PS1, Line 49: DCHECK(ExecEnv::GetQueryExecMgr() != nullptr);
I don't think we need this, but presumably it's redundant here if the 
constructor checked it exists.


PS1, Line 60: query
nit picking here, but it might be helpful to distinguish the query lifetime 
from the lifetime of the set of fragment instances; since the former can be 
significantly longer than the latter. Not sure what a good alternative is 
without introducing a new phrase for a group of fragment instances.


PS1, Line 65: Delete all query state in this class or accessible through this 
class.
Would just say it releases all resources owned by this class ("accessible 
through this class" sounds like it could reach out and delete other structures' 
data).


PS1, Line 69: /// Registers a new FInstanceState.
Either the comment or the method name should change. I would change the comment 
- the fact that an FInstanceState is registered seems like an implementation 
detail.


PS1, Line 73: Status
Under what conditions would this return an error?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I962ae6b7cb7dc0d07fbb8f70317aeb01d88d400b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to