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 <mar...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
