impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements
Date Fri, 18 Aug 2017 19:02:40 GMT
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 <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