Tim Armstrong has posted comments on this change. Change subject: IMPALA-4703: reservation denial debug action ......................................................................
Patch Set 3: (8 comments) Rebased onto my latest buffer pool preview patch and addressed comments. http://gerrit.cloudera.org:8080/#/c/7022/1/be/src/exec/exec-node.cc File be/src/exec/exec-node.cc: PS1, Line 452: T_ typo 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? Removed - missed removing this debugging code. 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 You're right, there was an off-by-one bug that was hit if rand() returns exactly RAND_MAX 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 I don't think it's actually necessary, so I removed it. 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 poin It was meant to be one-shot. I removed it because the purpose I had in mind no longer applied. 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 This still seems relevant since enabled() depends on this. Or did it seem redundant? PS3, Line 60: invalid > this is a bit confusing given that INVALID is a phase. maybe explain instea Done http://gerrit.cloudera.org:8080/#/c/7022/1/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: Line 70: // A floating point number in range [0.0, 1.0] that will be the probability of denying Wording. -- 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-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
