Dan Hecht has posted comments on this change. Change subject: IMPALA-4703: reservation denial debug action ......................................................................
Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/7022/3/be/src/exec/exec-node.cc File be/src/exec/exec-node.cc: Line 439: LOG(INFO) << "Debug action on " << id_; is that too much noise, or did you find it useful? Line 459: debug_action_param_.c_str(), debug_action_param_.size(), &parse_result); does "1.0" actually produce probability of exactly 1? i.e. is it guaranteed to always deny? http://gerrit.cloudera.org:8080/#/c/7022/3/be/src/runtime/bufferpool/buffer-pool.h File be/src/runtime/bufferpool/buffer-pool.h: Line 341: /// Call SetDebugDenyIncreaseReservation() on this client's ReservationTracker. explain the parameters. even after reading the code, I'm not 100% sure what you plan to do with delay. http://gerrit.cloudera.org:8080/#/c/7022/3/be/src/runtime/bufferpool/reservation-tracker.cc File be/src/runtime/bufferpool/reservation-tracker.cc: Line 154: if (increase_deny_delay_ == 0) { is increase_deny_delay_ meant to get reset to an initial count at this point, so that it kicks in again? Or is it meant to be one-shot? http://gerrit.cloudera.org:8080/#/c/7022/3/be/src/runtime/debug-options.h File be/src/runtime/debug-options.h: PS3, Line 55: // INVALID: debug options invalid this comment seems to be superseded by enable(), but action_param_ might be nice to explain. PS3, Line 60: invalid this is a bit confusing given that INVALID is a phase. maybe explain instead when 'true' is returned. -- To view, visit http://gerrit.cloudera.org:8080/7022 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ied39bb091b12156e5dc61b528c6c0cd8de3fe657 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-HasComments: Yes
