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 Sat, 22 Apr 2017 18:09:28 GMT
Marcel Kornacker has posted comments on this change.

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


Patch Set 9:

(26 comments)

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

PS9, Line 293: UpdateExecStats
> The similarity between UpdateExecStats() and UpdateExecStatus() is potentia
renamed.


Line 455:     // TODO: we don't track cpu time per node now. Do that.
> More generally we should have an aggregate profile for each backend that in
rephrased


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

Line 132:     const FInstanceExecParams& exec_params_;
> Might be helpful to document what owns the object being referenced.
Done


Line 164:   std::vector<const FInstanceExecParams*> instance_params_list_;
> Might be helpful to document what owns the objects being referenced.
Done


Line 175:   /// Protects fields below. Can be held while doing an RPC, so SpinLock is a bad
idea.
> Our current SpinLock implementation should be fine (it blocks after spinnin
Done


Line 219:   /// TODO: including the median doesn't compile, looks like some includes are missing
> This TODO has been in the code since 2012 - maybe just remove it?
Done


Line 253:   /// Root profile for all fragment instances for this fragment
> Stored in obj_pool?
Done


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

Line 364:           f->targets()->push_back(target);
> Maybe:
Done


Line 402:   int num_instances = schedule_.GetNumFragmentInstances();
> nit: intermediate variable seems unnecessary.
Done


Line 410:   // TODO: rename this metric
> There are a handful of minor TODOs like this. I assume there's some reason 
this was already taken care of


Line 938:   // Notify that we completed with an error.
> This comment seems wrong - the cancellation path is used to clean up even i
Done


PS9, Line 956:  
> nit: blank space
Done


Line 956:   // TODO: return here if returned_all_results_? 
> I believe we need to do cancellation even if we returned all results. Havin
i agree that we need to/should cancel as soon as we return all results. i left a corresponding
todo in the .h file (cleaning all that up as part of this change is out of scope).


Line 983:   // TODO: clarify control flow here, it's unclear we should even process this status
> Henry had a patch that clarified this and improved the comment: https://ger
i already incorporated that


PS9, Line 1136: VLOG_QU
> delete
Done


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

Line 108: /// TODO: clean up locking behavior; in particular, clarify dependency on lock_
> Does any of the locking information in the impala-server.h need to be updat
i didn't change the locking behavior, so no.


PS9, Line 151:  WARN_UNUSED_RESULT;
> nit: long line.
Done


Line 261:   boost::mutex wait_lock_;
> A lot of these locks could safely be SpinLocks - unless they're used with a
out of scope for this change, it's already large enough.


http://gerrit.cloudera.org:8080/#/c/6535/9/be/src/runtime/debug-options.cc
File be/src/runtime/debug-options.cc:

PS9, Line 71:  (*entry).
> entry->second ?
Done


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

Line 507: 
> Extra line
Done


http://gerrit.cloudera.org:8080/#/c/6535/9/be/src/runtime/fragment-instance-state.h
File be/src/runtime/fragment-instance-state.h:

Line 162:   /// True if and only if Cancel() has been called.
> I had a hard time reasoning about concurrent accesses to this until I reali
Done


http://gerrit.cloudera.org:8080/#/c/6535/9/be/src/runtime/query-exec-mgr.h
File be/src/runtime/query-exec-mgr.h:

Line 77:   /// Execute instances and decrement refcount.
> I find it clearer to understand as: "Acquires ownership of 'qs'".
Done


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

Line 43: #define RETRY_SLEEP_MS 100
> Make a class-level constant?
this will disappear with krpc


Line 311:   // don't return until every instance is prepared and record the first non-OK status
> Comment isn't really accurate - it doesn't record the first non-OK status b
Done


http://gerrit.cloudera.org:8080/#/c/6535/9/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 257:   void set_is_cancelled(bool v) { is_cancelled_ = v; }
> Not your change but related. This bool is accessed from multiple threads ye
we only ever change it from false to true, and in particular we don't do any compare&swap,
so i'm not sure how relevant synchronization is here.

i changed the signature to remove the bool parameter to make this clear.


http://gerrit.cloudera.org:8080/#/c/6535/9/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:

Line 96:   VLOG_QUERY << "FeSupport::NativeEvalExprs: " << texprs.size() <<
" exprs";
> Could get very noisy depending on how many constant exprs there are in the 
Done


-- 
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: 9
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