Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10058 )
Change subject: IMPALA-6847: work around high memory estimates for AC ...................................................................... Patch Set 11: (6 comments) Here's the new simplified version that just changes the estimate sent from the frontend to backend. http://gerrit.cloudera.org:8080/#/c/10058/8/be/src/scheduling/query-schedule.cc File be/src/scheduling/query-schedule.cc: http://gerrit.cloudera.org:8080/#/c/10058/8/be/src/scheduling/query-schedule.cc@194 PS8, Line 194: DCHECK(request_.__isset.per_host_mem_estimate); > Could you add a comment here about this case, maybe something like: Reverted this part of the change. http://gerrit.cloudera.org:8080/#/c/10058/8/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/10058/8/common/thrift/Frontend.thrift@414 PS8, Line 414: : enum TCatalo > garbled This is no longer needed for the latest patch. http://gerrit.cloudera.org:8080/#/c/10058/8/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/10058/8/common/thrift/ImpalaInternalService.thrift@283 PS8, Line 283: enabled, i. > maybe delete that, since it can be set for the pool or globally (which then Done http://gerrit.cloudera.org:8080/#/c/10058/8/common/thrift/ImpalaInternalService.thrift@285 PS8, Line 285: ISSION, : // planner memory estimate) is used for admission control purposes. This provides a > this seems a bit ambiguous. Protection from what? From not being admitted, Removed since it no longer applies to latest patch. http://gerrit.cloudera.org:8080/#/c/10058/8/common/thrift/ImpalaInternalService.thrift@441 PS8, Line 441: ms { > resolving? This field is no longer needed with the new code. http://gerrit.cloudera.org:8080/#/c/10058/8/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/10058/8/common/thrift/ImpalaService.thrift@295 PS8, Line 295: _MEM_ESTIMATE_FOR_ADMISSION, > let's copy that to the other place I just added a reference to the other place where the test is. Maintaining two identical comments seems pointless. -- To view, visit http://gerrit.cloudera.org:8080/10058 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia5fc32a507ad0f00f564dfe4f954a829ac55d14e Gerrit-Change-Number: 10058 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 17 Apr 2018 00:33:31 +0000 Gerrit-HasComments: Yes
