Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4014: Introduce query-wide execution state. ......................................................................
Patch Set 5: (11 comments) http://gerrit.cloudera.org:8080/#/c/4418/5/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS5, Line 938: || > joining condition should go before the line break? no http://gerrit.cloudera.org:8080/#/c/4418/5/be/src/runtime/query-exec-mgr.cc File be/src/runtime/query-exec-mgr.cc: PS5, Line 72: qs->refcnt_.Add(1); > If this is the logic we're going with, I would suggest that this patch and i'm not sure what you're talking about. the qs is reused for subsequent fragment instances, unless you have some corner case (e.g., first instance fails and gc's qs). PS5, Line 125: int32_t cnt = qs->refcnt_.Add(1); > After we have a per-query RPC, I think we should disallow getting the QS if why? it's not just the execution threads that need qs references. PS5, Line 135: qs->refcnt_.Load() > I think it will be better to put this LOG after getting the local 'cnt' val Done Line 141: > One problem I see with this is, every time we hit refcount=0, there is some i'm not sure what you mean by 'unnecessarily'. the qs is gc'd when the refcnt goes to 0. i don't see a problem with another async thread increment the refcnt again, given that that same thread will eventually also decrement it. why would that be a problem? http://gerrit.cloudera.org:8080/#/c/4418/5/be/src/runtime/query-exec-mgr.h File be/src/runtime/query-exec-mgr.h: PS5, Line 66: /// lock order: : /// 1. qs_map_lock_ : /// 2. QueryState::refcnt_lock_ > Remove now that refcnt is an AtomicInt Done PS5, Line 69: boost::mutex qs_map_lock_ > This will soon need to be a RWlock, or it could introduce scaling issues. B i don't think it'll cause scaling issues, at least not at typical qps levels, but i'll let mostafa look into it. http://gerrit.cloudera.org:8080/#/c/4418/5/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: PS5, Line 338: local_query_ctx_ > If we're going to have a local_query_ctx_ anyway, why bother with accessing the .h file points out that it's only used when there's no querystate, ie, for tests. we definitely do not want to make a gratuitous copy of the queryctx, which can be a *very* large structure. http://gerrit.cloudera.org:8080/#/c/4418/5/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: PS5, Line 49: class ExecEnv; : class DataStreamMgr; : class HBaseTableFactory; : class TPlanFragmentCtx; : class TPlanFragmentInstanceCtx; : class QueryState; > Alphabetical order? that's immaterial to correctness or comprehensibility. PS5, Line 329: // needed? : // static const int DEFAULT_BATCH_SIZE = 1024; > Can remove? since its moved to query-state.h Done PS5, Line 351: TUniqueId no_instance_id_; > Who sets this? It doesn't seem to be used now. My guess is it should be set it's initialized to an invalid id and used in l127 of this file. -- 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: 5 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-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-HasComments: Yes
