Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8781 )
Change subject: IMPALA-6222: Add details to error msg on failure to get min reservation ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/bufferpool/reservation-tracker.cc File be/src/runtime/bufferpool/reservation-tracker.cc: http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/bufferpool/reservation-tracker.cc@174 PS1, Line 174: *error_status = Status::Expected(Substitute( > I was thinking of adding it to generate_error_codes.py but that would mean This seems ok then. http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/bufferpool/reservation-tracker.cc@177 PS1, Line 177: he top 5 queries " : "that allocated memory under this tracker are:\n$6", > would it make more sense to have the top 5 queries based on reservation rat I think just sorting them by consumption is good enough. We should be moving more things to reservation anyway so the difference will get smaller over time. http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/bufferpool/reservation-tracker.cc@185 PS1, Line 185: memtracker->LogTopNQueries(5 > I see what you mean, I think error message would make more sense if I only Yeah that makes sense to me. http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/bufferpool/reservation-tracker.cc@199 PS1, Line 199: Substitute("Failed to increase reservation by $0 because it would exceed the " > We should probably stick to using this over MemLimitExceeded as this one is WFM http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/bufferpool/reservation-tracker.cc@203 PS1, Line 203: mem_tracker_->LogUsage(0), mem_tracker_->LogTopNQueries(5))); > TryConsumeFromMemTracker only returns false if the memtracker has no parent That's not quite right. E.g. if this is the query reservation tracker, TryConsume() on the query mem tracker will check both the query and process limits. The subtlety is that GetParentMemTracker() returns NULL if this reservation tracker's parent is not linked to a mem tracker. That doesn't actually imply that the linked mem tracker does not have a parent. I.e. parent_->mem_tracker_ can be null even if mem_tracker_->parent_ is non-NULL. Maybe the name of the function could be improved... -- To view, visit http://gerrit.cloudera.org:8080/8781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic4675fe923b33fdc4ddefd1872e6d6b803993d74 Gerrit-Change-Number: 8781 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 11 Dec 2017 22:18:57 +0000 Gerrit-HasComments: Yes
