Bikramjeet Vig 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: (11 comments) 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? Done 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 th Done 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 Done 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 d I was thinking of adding it to generate_error_codes.py but that would mean that I wont be able to use Status::expected here to avoid printing the stacktrace. Also, i will be breaking this error message as described in the following comment. 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 rather than memory consumption, I can add a flag to LogTopNQueries() to sort by amount of reservation instead. Or will this extension of LogTopNQueries() be superfluous? 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 reserv I see what you mean, I think error message would make more sense if I only append the "The top 5 queries..." part only if memtracker->LogTopNQueries(5) returns a non empty string (since it only returns something if it is executed for query mem trackers and 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 w Done 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::M We should probably stick to using this over MemLimitExceeded as this one is more descriptive. 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, unfor TryConsumeFromMemTracker only returns false if the memtracker has no parent and hence no ancestors. So if the consume call fails, we can be sure that it is the same memtracker 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 guarant Done 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 Done -- 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: Sat, 09 Dec 2017 02:31:00 +0000 Gerrit-HasComments: Yes
