Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201609170036.u8H0aTcd006724@ip-10-146-233-104.ec2.internal>
Date: Sat, 17 Sep 2016 00:36:29 +0000
From: "Henry Robinson (Code Review)" <gerrit@cloudera.org>
To: Marcel Kornacker <marcel@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mj@cloudera.com>, Alex Behm <alex.behm@cloudera.com>,
        Lars Volker <lv@cloudera.com>
Reply-To: henry@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4014=3A_HEADERS_ONLY=3A_Introduce_query-wide_execution_state=2E=0A?=
In-Reply-To: <gerrit.1473900487000.I962ae6b7cb7dc0d07fbb8f70317aeb01d88d400b@gerrit.cloudera.org>
References: <gerrit.1473900487000.I962ae6b7cb7dc0d07fbb8f70317aeb01d88d400b@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Sat, 17 Sep 2016 00:36:36 -0000

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 <marcel@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes