impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Date Wed, 30 Nov 2016 21:48:12 GMT
Marcel Kornacker has posted comments on this change.

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

Patch Set 5:

File be/src/runtime/

PS5, Line 938: ||
> joining condition should go before the line break?
File be/src/runtime/

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

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?
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

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.
File be/src/runtime/

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.
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

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I962ae6b7cb7dc0d07fbb8f70317aeb01d88d400b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message