impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
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:

(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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message