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-2550: Switch to per-query exec rpc
Date Tue, 18 Apr 2017 01:31:25 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-2550: Switch to per-query exec rpc

Patch Set 6:

File be/src/common/status.h:

Line 22: #include <ostream>
> Can you include iosfwd instead. status.h is included everywhere and it woul
File be/src/exec/

PS4, Line 66:   
> We use indentation 4 for line continuation. See line 119 below.
we use 4 spaces for the first line and 2 spaces subsequently for a multi-line statement.
File be/src/runtime/

PS6, Line 46: using boost::lock_guard;
            : using boost::mutex;
> #include "common/names.h" for these.

PS6, Line 348: true
> the class comment says that Cancel() returns true if the cancellation happe
changed comment. this is only informational anyway.
File be/src/runtime/coordinator-backend-state.h:

PS4, Line 97:  boost::mutex* lock() { return &lock_; }
> Is it possible to not expose the lock ? It'd be easier to reason about the 

PS4, Line 181:   /// If the status indicates an error status, execution has either been aborted
by the
             :   /// executing impalad (which then reported the error) or cancellation has
             :   /// initiated; either way, execution must not be cancelled.
             :   Status status_;
> Why the two separate data structures ? Why cannot this be a map of instance
File be/src/runtime/

PS2, Line 252: reate BackendStates
             :   bool has_coord_fragment = schedule_.GetCoordFragment() != nullptr;
             :   const TNetworkAddress& coord_address = ExecE
> doesn't need to be two statements.

PS2, Line 262: kend_state->Init(entry.second, fra
> const &?

PS2, Line 264: 
> This would be better named coord_backend_idx_.

Line 977:               << "\n" << s.str();
henry, is there any reason to keep this?

PS2, Line 1000:   // debugging aid for back
> Acquiring this lock is problematic for an RPC handler. See <TODO>.
true, but i don't want to extend the scope of this change to fix locking problems as well.
File be/src/runtime/

PS6, Line 408: // TODO: rename this metric
> Why not do this now? (But note that for the moment that name isn't publishe
meaning nothing depends on this name at the moment?

PS6, Line 415: || status.IsCancelled()
> When would we have a backend that was cancelled at this point (since cancel
good point, reverted.

PS6, Line 858: Report aggregate
             :   // query profiles at this point.
> This comment seems out-of-sync with the rename of ReportQuerySummary().

PS6, Line 885: TODO: do we need this error propagation path, in addition to the status report
             :   // we'll get anyway?
> This error is needed so that client-facing operations can fail-fast. The ne
when a fragment instance fails it reports an error status right away.

i don't see any harm in leaving that todo in here.

PS6, Line 986: if (!(returned_all_results_ && status.IsCancelled()) && !status.ok())
> Will you include the fix for IMPALA-4925 here as discussed? (see https://ge
if returned_all_results_ is true, it's questionable whether we shouldn't simply return at
the topic (since the query summary has already been computed).

i'm happy to include the "fix", but i think these control paths need more work.

PS6, Line 993: lock_guard<mutex> l(lock_);
> We spoke offline about removing this lock acquisition in this patch (see ht
let's do it as a follow-on, it's purely additive.
File be/src/runtime/

PS6, Line 199: // TODO: what to do with this? should we treat a failure to get a backend
             :     // connection as a failure of instance execution?
> Yes. 
File be/src/runtime/query-state.h:

PS6, Line 65:  - guarantee delivery of the ReportExecStatus rpc in case of an error, otherwise
            : ///   the query might hang
> We can't guarantee delivery - not quite sure what you mean here. 
File be/src/runtime/runtime-state.h:

Line 340:   /// only populated by test c'tor
> Isn't it also used by the fe-support code path?
File be/src/runtime/

Line 23: #include "common/names.h"
> common/names.h usually gets put below the other includes because it checks 

Line 61:   if (buffer_pool_min_buffer_len_ != -1 && buffer_pool_capacity_ != -1) {
> Thanks for fixing this.
File common/thrift/ImpalaInternalService.thrift:

PS4, Line 399: across 
> So, is this a per fragment index different from the globally unique instanc

PS4, Line 400: num_fragment_instances
> I don't see a field called num_fragment_instances in TPlanFragmentCtx defin

PS4, Line 401: per_
> Why per_ ? The comment calls it fragment_instance_idx.
which comment?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I20769e420711737b6b385c744cef4851cee3facd
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message