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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
