Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4014: Introduce query-wide execution state. ......................................................................
Patch Set 4: (18 comments) http://gerrit.cloudera.org:8080/#/c/4418/4/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS4, Line 502: QueryState* qs = ExecEnv::GetInstance()->query_exec_mgr()->GetQueryState(query_id_); > ScopedQueryState ? that's a bug: you need to keep the reference until the query completes. see the call to ReleaseQueryState() in TearDown(). http://gerrit.cloudera.org:8080/#/c/4418/4/be/src/runtime/query-exec-mgr.cc File be/src/runtime/query-exec-mgr.cc: PS4, Line 59: boost:: > remove Done Line 66: VLOG_QUERY << "new QueryState: query_id=" << query_id; > Why is there no refcnt increment here? it's a new one, and the refcnt is initialized to 1 (documented for the c'tor). PS4, Line 69: boost:: > remove here and below. Done PS4, Line 86: Thread::Run("query-exec-mgr", : Substitute("exec-fragment-instance-$0", PrintId(instance_id)), : &QueryExecMgr::StartFInstanceHelper, this, fis); > Why is this not in the QueryState? It seems like it should be part of Regis i felt both approaches are equally valid. PS4, Line 95: StartFInstanceHelper > Call this ExecFInstance() to make it clear that it blocks until instance co Done PS4, Line 115: ReleaseQueryState(fis->query_state()); > You have a bug here - if a fragment instance finishes quickly (perhaps it d i thought about this, but didn't see it as a bug. why do you think it's a bug? the second fragment instance will simply re-create the qs. PS4, Line 124: boost::lock_guard<mutex> l2(qs->refcnt_lock_); : ++qs->refcnt_; > Prefer atomics rather than heavyweight mutexes for the ref count. Fewer loc Done PS4, Line 126: DCHECK_GT(qs->refcnt_, 0); > What are you checking for here - overflow or that the refcnt_ didn't start semantically it's the same to check it here, and this requires less synchronization. PS4, Line 147: // someone else might have gc'd the entry > How? Only one caller to ReleaseQueryState() can set refcnt == 0. after decreasing the refcnt to 0 in l138 someone else could have increased it again before we get the lock in l145. http://gerrit.cloudera.org:8080/#/c/4418/4/be/src/runtime/query-exec-mgr.h File be/src/runtime/query-exec-mgr.h: PS4, Line 47: In both cases it brackets the instance execution with an increment/decrement : /// of the refcount. > This needs clarification - why is this relevant to callers? I think you mea it expresses that the qs won't go away while the instance is executing. PS4, Line 58: Otherwise returns nullptr > AFAICT, GetQueryState() will return a QueryState even if its refcount == 0. i'll change this comment to fit the existing behavior. i like the existing behavior better, because it eliminates the possibility of someone trying to register a qs because this function returned nullptr, despite the qs still being accessible. PS4, Line 66: /// TODO: isn't this available in std:: now? > remove, once we switch to std::mutex we'll get this one as well. Done PS4, Line 72: QueryState (owned by us) > Comment on how it's allocated. you mean that it's calling new? PS4, Line 75: /// Execute instance and clean up. > Clean up what? Does this block until instance has finished? Done http://gerrit.cloudera.org:8080/#/c/4418/4/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: PS4, Line 42: refcnt_(1) > set this to 0. Callers should always manage the refcount with a balanced nu Done http://gerrit.cloudera.org:8080/#/c/4418/4/be/src/runtime/query-state.h File be/src/runtime/query-state.h: PS4, Line 79: /// don't access query_ctx().desc_tbl, it's most likely for a different fragment instance > I don't understand this comment. Is this going to change after batching? Ot yes, this will change with the per-query exec rpc. it's broken right now because every fragment instance shows up with its own descriptor table, and they're all slightly different. PS4, Line 107: bool > Does access to this need to be synchronised? not so far. -- 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: 4 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: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-HasComments: Yes
