Tim Armstrong has posted comments on this change. Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements ......................................................................
Patch Set 6: Code-Review+2 (5 comments) http://gerrit.cloudera.org:8080/#/c/7678/5/be/src/runtime/bufferpool/reservation-util.h File be/src/runtime/bufferpool/reservation-util.h: Line 41: /// (i.e. not accounted by buffer reservation trackers) stays within This TODO is inconsistent with the others. I dont' think it really matters though. http://gerrit.cloudera.org:8080/#/c/7678/6/be/src/runtime/bufferpool/reservation-util.h File be/src/runtime/bufferpool/reservation-util.h: Line 18: // This file includes constants used for buffer management. Maybe remove or combine with class comment? Line 30: /// There are currently two classes of memory: reserved memory (i.e. memory that is This is a big improvement. Line 37: /// determine how much memory will get used for reserved memory vs unreserved memory. The last part sounds like we're partitioning between the two, but we're really just bounding reserved memory. Maybe instead end with "...used to determine an upper bound on reserved memory. Operators are designed to operate reliably when they are up against a hard constraint on reserved memory (e.g. staying under a limit by spilling), but will generally fail if they hit a limit when trying to allocate unreserved memory. Thus we need to ensure there is always space left in the query memory limit for unreserved memory." PS6, Line 64: hueristic heuristic -- To view, visit http://gerrit.cloudera.org:8080/7678 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
