Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements ......................................................................
Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/7678/5/be/src/runtime/bufferpool/reservation-util.h File be/src/runtime/bufferpool/reservation-util.h: PS5, Line 32: allocated to buffer reservations, > After thinking about these variables more, this paragraph doesn't seem righ I think it's the latter. Maybe it'd be more clear if the first sentence was changed to: The fraction of the query mem limit that is used as the maximum buffer reservation limit. It is expected that memory not accounted by buffer reservation trackers stays within (1 - RESERVATION_MEM_FRACTION). I think the part of the text that you highlighted is just explaining the motivation for having a high value. What do you/Tim think? PS5, Line 36: : /// The limit on reservations is co > I don't follow that. we don't use buffer reservations for those yet. I think this is providing examples of things that use the memory that is left after buffer reservations. I agree that depending on how you read it, it could be misleading. I can reword this to try to be more clear. I incorporated this text from Tim's change to reduce RESERVATION_MEM_MIN_REMAINING, so it'd be good for him to comment as well. Tim, is there any comment anywhere else about the 'classes' of memory already? http://gerrit.cloudera.org:8080/#/c/7678/4/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: Line 125: "more information."; > I think we should soon make it so you can't disable admission control. Not Ok, I filed IMPALA-5814 for that. PS4, Line 429: GetBufferReservationLimitFromMemLimit > I missed that. But should line 425 be an else-if then? This was actually my intention originally, and that's why I asked the question that I did in query-state.cc. I think it makes sense to apply both limits, but if we won't do that in query-state.cc then I'll make this code match. -- 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: 4 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
