impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
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:

(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 <mj@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message