Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4833: Compute precise per-host reservation size ......................................................................
Patch Set 8: (6 comments) Good questions/comments. I think I addressed the comments, though I'm still not terribly happy with all the names. I'll post a separate review since this already went in. 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 Done Line 57: // instance_params. > I think this comment lost some information compared to the equivalent comme Done PS8, Line 62: initial_reservation_total_bytes > I found that having the word "claim" in the old equivalent members made it Done 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 t This is still used for the explain output. I don't think there's a way to get the 'tighter' numbers there without going through scheduling. That said, it's pretty unfortunate that this field exists given it gets computed in Planner.computeResourceReqs() and gets used in Planner.getExplainString(). 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 doe Done PS8, Line 490: initial_reservation_total_bytes > same comment Done -- 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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
