impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4703: reservation denial debug action
Date Wed, 21 Jun 2017 23:21:51 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4703: reservation denial debug action

Patch Set 3:


Rebased onto my latest buffer pool preview patch and addressed comments.
File be/src/exec/

PS1, Line 452: T_
File be/src/exec/

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
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.
File be/src/runtime/bufferpool/

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.
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
File common/thrift/PlanNodes.thrift:

Line 70:   // A floating point number in range [0.0, 1.0] that will be the probability of

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ied39bb091b12156e5dc61b528c6c0cd8de3fe657
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message