impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements
Date Fri, 18 Aug 2017 17:58:37 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements

Patch Set 4:

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 -

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?
File be/src/scheduling/

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 I think it makes sense to apply both limits, but if we won't do that in then I'll make this code match.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message