impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
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:

(25 comments)

http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/common/status.h
File be/src/common/status.h:

Line 22: #include <ostream>
> Can you include iosfwd instead. status.h is included everywhere and it woul
Done


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/exec/data-sink.cc
File be/src/exec/data-sink.cc:

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.


http://gerrit.cloudera.org:8080/#/c/6535/6/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

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


PS6, Line 348: true
> the class comment says that Cancel() returns true if the cancellation happe
changed comment. this is only informational anyway.


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/coordinator-backend-state.h
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 
Done


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
been
             :   /// initiated; either way, execution must not be cancelled.
             :   Status status_;
> Why the two separate data structures ? Why cannot this be a map of instance
Done


http://gerrit.cloudera.org:8080/#/c/6535/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

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


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


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


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.


http://gerrit.cloudera.org:8080/#/c/6535/6/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

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().
Done


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.


http://gerrit.cloudera.org:8080/#/c/6535/6/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

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


http://gerrit.cloudera.org:8080/#/c/6535/6/be/src/runtime/query-state.h
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. 
Done


http://gerrit.cloudera.org:8080/#/c/6535/6/be/src/runtime/runtime-state.h
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?
Done


http://gerrit.cloudera.org:8080/#/c/6535/6/be/src/runtime/test-env.cc
File be/src/runtime/test-env.cc:

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


Line 61:   if (buffer_pool_min_buffer_len_ != -1 && buffer_pool_capacity_ != -1) {
> Thanks for fixing this.
Done


http://gerrit.cloudera.org:8080/#/c/6535/4/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

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


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


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


-- 
To view, visit http://gerrit.cloudera.org:8080/6535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I20769e420711737b6b385c744cef4851cee3facd
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message