Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10158 )
Change subject: IMPALA-5384, part 2: Simplify Coordinator locking and clarify state ...................................................................... Patch Set 14: (33 comments) http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@23 PS14, Line 23: #include <boost/accumulators/accumulators.hpp> > Not your change but these accumulators don't look like they're used. moved to coordinator-backend-state.h, where they are used. http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@33 PS14, Line 33: #include <boost/unordered_set.hpp> > Not used? Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@37 PS14, Line 37: #include "common/hdfs.h" > Move to .cc? Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@42 PS14, Line 42: #include "runtime/query-state.h" > I think you could move this to the .cc - QueryState is already forward-decl to .cc and coordinator-backend-state.h http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@43 PS14, Line 43: #include "scheduling/query-schedule.h" > Could forward-declare QuerySchedule and move this to the .cc. query_ctx() was relying on that so moved that to .cc (and moved query_id() back). http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@87 PS14, Line 87: /// 1. When an error is encountered while still executing, backend cancellation is > Can we define what "executing" means? In this context it just means hasn't encountered either of these three conditions. I've tried to clarify by rewording. http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@99 PS14, Line 99: /// > Should we document the lock order? Coordinator::lock_ is mentioned in the I I deleted that impala-server.h comment, lmk if you disagree. Added short comment about ordering. http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@114 PS14, Line 114: > extra spaces in this comment Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@130 PS14, Line 130: /// Cancel execution of query and sets the overall query status to CANCELLED if the > Long line Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@191 PS14, Line 191: accesses > accessing. Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@214 PS14, Line 214: boost::mutex wait_lock_; > Switch to SpinLock? The inconsistency might be confusing otherwise. Probabl Let me do that in a follow up commit. It should be fine but could have some weird subtle side effect. http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@218 PS14, Line 218: /// Keeps track of number of completed ranges and total scan ranges. Initialized by Exec(). > Long line. Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@219 PS14, Line 219: ProgressUpdater progress_; > I don't feel that strongly but it seems like we could PIMPL this and avoid Given you don't feel strongly (nor do I) and it's not related to this change and the change is large enough (in terms of complexity) let's defer that for now. http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@291 PS14, Line 291: ExecState exec_state_ = ExecState::EXECUTING; > It's a little confusing that this is EXECUTING before Exec() is called. The I agree it's a little confusing, but there is nothing you can do with this object between construction and calling Exec() so not sure it's worth making a distinct state (the implementation is also simplified given that all transformations result in a terminal state). Maybe I can clarify with comments -- Which part are you referring to about the distinction? http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@325 PS14, Line 325: /// If 'exec_state_' != EXECUTING, does nothing. Otherwise sets 'exec_state_' to > Maybe a brief high-level description would be helpful. E.g. "Called when th Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@326 PS14, Line 326: /// 'state' (must be either CANCELLED or RETURNED_RESULTS), > Formatting of this comment seems weird. not sure what happened there. fixed. http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@333 PS14, Line 333: /// Transitions 'exec_state_' given an execution status: > It's a little hard to decode why someone would call this with an OK status I moved the last line up to the beginning. Hopefully that helps clarify. http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@351 PS14, Line 351: > extra space Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@360 PS14, Line 360: void > void argument list not needed in C++ - () is equivalent to (void) unlike C. yeah, old habits die hard http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@406 PS14, Line 406: > extra space another old habit, done. http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.cc@54 PS14, Line 54: using namespace strings; > strings "using" may not be needed Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.cc@61 PS14, Line 61: using std::unique_ptr; > unique_ptr "using" may not be needed Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.cc@63 PS14, Line 63: DECLARE_int32(be_port); > be_port not used. Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.cc@79 PS14, Line 79: teriminal > terminal Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.cc@449 PS14, Line 449: if (exec_state_ != ExecState::EXECUTING) return exec_status_; > Worth defining an "is_terminal_state()" helper? I don't think we'd use it anywhere else, so my preference would be leave it along. lmk if you feel strongly. http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.cc@477 PS14, Line 477: DCHECK(exec_status_.ok()); > Maybe log the status if this DCHECK fails? Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.cc@481 PS14, Line 481: DCHECK(exec_status_.IsCancelled()); > Maybe log the status if this DCHECK fails? Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.cc@486 PS14, Line 486: DCHECK(!exec_status_.ok()); > Maybe log the status if this DCHECK fails? for this one, it means status.ok() if it fails. http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.cc@517 PS14, Line 517: // Once we enter a terminal state, we stay there. That guarantees this code runs only once. > Long line. Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.cc@526 PS14, Line 526: temrinal > terminal Done http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.cc@540 PS14, Line 540: // TODO: should move this off of the query execution path? > Is there a JIRA for this? Not yet. I wanted to first determine what expectations clients have on the profile being complete for queries that finish without errors. http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.cc@623 PS14, Line 623: TOOD > "TODO". It's also unclear what this TODO depends on Inspection of the ClientRequestState code. I think it's already true, but it's not trivial to see without some cleanup there. I did run tests with a dcheck in this if-stmt and it doesn't get hit, but wanted to go back in git to see what case this branch was added for. I'll just remove the TODO for now, it was move of a reminder to myself but then I decided not to change the code here in this commit. http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.cc@735 PS14, Line 735: for (BackendState* state: backend_states_) { > nit: one line Done -- To view, visit http://gerrit.cloudera.org:8080/10158 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1abdfd02163f9356c59d470fe1c64ebe012a9e8e Gerrit-Change-Number: 10158 Gerrit-PatchSet: 14 Gerrit-Owner: Dan Hecht <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 09 May 2018 03:05:18 +0000 Gerrit-HasComments: Yes
