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: (10 comments) I like the overall approach and the tests. Comments are mainly about the invariants we can assume about where limits are placed in the hierarchy. http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/bufferpool/reservation-tracker-test.cc File be/src/runtime/bufferpool/reservation-tracker-test.cc: http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/bufferpool/reservation-tracker-test.cc@44 PS1, Line 44: NULL nullptr? http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/bufferpool/reservation-tracker.h File be/src/runtime/bufferpool/reservation-tracker.h: http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/bufferpool/reservation-tracker.h@121 PS1, Line 121: /// 'error_status'. Maybe add something like (if 'error_status' is non-NULL) just to clarify that it's valid to pass in NULL? 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@168 PS1, Line 168: memtracker nit: mem_tracker for consistency http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/bufferpool/reservation-tracker.cc@174 PS1, Line 174: *error_status = Status::Expected(Substitute( Seems like we should probably add this one to generate_error_codes.py (we don't have terribly consistent criteria for doing so but it's usually a good practice). http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/bufferpool/reservation-tracker.cc@185 PS1, Line 185: memtracker->LogTopNQueries(5 This is kind of assuming that only query mem trackers and above have reservation limits - this would produce a weird result if we called it on a lower one. The assumption is true for now but I'm wary of adding an undocumented assumption here since we might want to add constraints to other plan nodes, e.g. for partial sort. I guess we could document on this function that the error message only makes sense if you call this on a query-level reservation tracker (or above). http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/bufferpool/reservation-tracker.cc@185 PS1, Line 185: memtracker->LogTopNQueries(5))); I feel like we should probably document the lock order implied here since we're calling a function that acquires MemTracker::child_trackers_lock_ while holding ReservationTracker::lock_. Maybe document in the ReservationTracker::lock_ comment? 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 " Also move to generate_error_codes.py? Or maybe we should call MemTracker::MemLimitExceeded() here instead for consistency? 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))); We don't actually know that the limit hit was on mem_tracker_ itself, unfortunately, it could be further up. I think calling MemTracker::MemLimitExceeded() might solve this issue though. http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/mem-tracker.cc@336 PS1, Line 336: void MemTracker::GetTopNQueries(std::priority_queue<MemTracker*, vector<MemTracker*>, Returning the pointers to the MemTrackers isn't safe - they're only guaranteed to live as long as 'child_trackers_lock_' is held. I think we need to copy out any state from the child trackers that we need before dropping the lock. http://gerrit.cloudera.org:8080/#/c/8781/1/be/src/runtime/mem-tracker.cc@338 PS1, Line 338: { nit: curly braces don't seem necessary since the lock is held for the whole function. -- 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: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 06 Dec 2017 23:45:15 +0000 Gerrit-HasComments: Yes
