Dan Hecht has posted comments on this change. Change subject: IMPALA-4833: Compute precise per-host reservation size ......................................................................
Patch Set 8: (6 comments) http://gerrit.cloudera.org:8080/#/c/7630/8/be/src/scheduling/query-schedule.h File be/src/scheduling/query-schedule.h: PS8, Line 56: fragments instances Line 57: // instance_params. I think this comment lost some information compared to the equivalent comments in the old code. From the comment, it's hard to know whether this is the minimum over all instances of the reservation, or the total of all the minimums. It would help to explain what this is conceptually. PS8, Line 62: initial_reservation_total_bytes I found that having the word "claim" in the old equivalent members made it easier to keep track of what this was, since "total" can be so vague. http://gerrit.cloudera.org:8080/#/c/7630/8/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: PS8, Line 399: in the case all fragments are scheduled on all hosts with : // max DOP given that this patch is addressing the overestimation, why do we compute this overestimate? http://gerrit.cloudera.org:8080/#/c/7630/8/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: PS8, Line 483: fragments in fragment_ctxs is it over fragments or instances? if fragments and not instances, how does that work out? PS8, Line 490: initial_reservation_total_bytes same comment -- To view, visit http://gerrit.cloudera.org:8080/7630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idbcd1e9b1be14edc4017b4907e83f9d56059fbac Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
