Marcel Kornacker has posted comments on this change. Change subject: IMPALA-3610: Account for memory used by filters in the coordinator ......................................................................
Patch Set 5: (10 comments) http://gerrit.cloudera.org:8080/#/c/4066/5/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: Line 284: /// coarsley for every filter update. remove "coarsley" (sic) Line 289: // Apply an update to the aggregated filter with the received filter. describe role of params Line 292: TPublishFilterParams* rpc_params, const TUniqueId& query_id); in/out params go at end Line 343: // logging. what do the scanner threads do with this mem tracker? Line 2102: TPublishFilterParams* rpc_params, const TUniqueId& query_id) { pass in Coordinator* instead of the mem tracker and query_id, that's more compact. Line 2103: filter_updates_received->Add(1); move this out into the caller, there is no other use of this parameter (and it doesn't do anything to the filter state anyway). Line 2105: if (!desc.is_broadcast_join) { if (is_broadcast_join) return; Line 2115: disabled = true; why don't you move this into Discard and then check that explicitly in UpdateFilter()? that would have made the bug more obvious. Line 2131: // 'bloom_filter' will process subsequent updates otherwise. the reason this wasn't apparent is that it wasn't stated what a discarded/invalid filter state looks like. augment the struct definition (is there a reason why you'd want to distinguish between pending count == 0 and invalid? if so, state it.) http://gerrit.cloudera.org:8080/#/c/4066/4/be/src/util/bloom-filter.h File be/src/util/bloom-filter.h: Line 82: void Or(const BloomFilter& other); > No it isn't. Only tests use it now. in that case, please remove it. -- To view, visit http://gerrit.cloudera.org:8080/4066 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
