Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 )
Change subject: IMPALA-5519: Allocate Runtime filter memory from Buffer pool ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/8971/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8971/3//COMMIT_MSG@12 PS3, Line 12: > Maybe mention that it doesn't address the coordinator memory problem? Should we be adding any context related to the coordinator problem here? I believe adequately mentioning that problem here might take a couple of extra sentences that would take the focus off of the core patch which is related to fragments only. http://gerrit.cloudera.org:8080/#/c/8971/3/be/src/util/bloom-filter.h File be/src/util/bloom-filter.h: http://gerrit.cloudera.org:8080/#/c/8971/3/be/src/util/bloom-filter.h@112 PS3, Line 112: directoty_ > typo: directory Done http://gerrit.cloudera.org:8080/#/c/8971/3/be/src/util/bloom-filter.cc File be/src/util/bloom-filter.cc: http://gerrit.cloudera.org:8080/#/c/8971/3/be/src/util/bloom-filter.cc@50 PS3, Line 50: Close(); > Calling Close() is a little subtle. Maybe mention the reason for doing this Done http://gerrit.cloudera.org:8080/#/c/8971/3/fe/src/main/java/org/apache/impala/planner/PlanFragment.java File fe/src/main/java/org/apache/impala/planner/PlanFragment.java: http://gerrit.cloudera.org:8080/#/c/8971/3/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@263 PS3, Line 263: .sum(planTreeProfile.duringOpenProfile.max(fInstancePostOpenProfile)); > I missed this on the first pass. I think we need to add the runtime filters I think this would achieve the same. Just takes the max of duringOpenProfile and fInstancePostOpenProfile before adding the runtime filters reservation http://gerrit.cloudera.org:8080/#/c/8971/3/testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test File testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test: http://gerrit.cloudera.org:8080/#/c/8971/3/testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test@59 PS3, Line 59: partitions=24/24 files=24 size=174.39KB > Changes in this file look spurious. these sizes seem to change across test runs, maybe due to changes in data loading. these seemed spurious to me too but then i looked at the logs of some core test runs on jenkins and they seemed to be different too. Moreover, core jobs close to my last rebase of this patch are exactly equal to this value. -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 18 Jan 2018 01:54:55 +0000 Gerrit-HasComments: Yes
