Dan Hecht has posted comments on this change. Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements ......................................................................
Patch Set 5: Code-Review+2 (4 comments) Code looks fine but I think those (pre-existing) comments could use some fixing/clarifying. I'll be on PTO tomorrow, so feel free to work with Tim to get that finished up (or I can look on Monday). 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 right. does this amount actually get set aside to be used only be the buffer pool (i.e. allocated to buffer reservations), or does it just set the upper bound on the amount of buffer pool memory that the query can use? PS5, Line 36: or : /// scans, exprs, row batches, etc. I don't follow that. we don't use buffer reservations for those yet. Oh, do you mean the amount of memory (for scans, exprs, row batches, etc) that should not be usable by the buffer pool? This comment applies to both of these variables though, right? So maybe we should have a quick high level comment explaining that memory is divided into two classes and what they are and examples of how they're used, and then the variable specific comments can just explain how they put guard rails on each so neither is starved. 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."; > Yeah this overall seemed simplest since at this point we've already resolve I think we should soon make it so you can't disable admission control. Not sure that even has to wait for 3.0 if the default configuration is logically equivalent (other then fail fast vs fail slow). PS4, Line 429: GetReservationLimitFromMemLimit(mem_l > Do you mean something besides what gets handled on l416-l424? I missed that. But should line 425 be an else-if then? -- 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: 5 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
