Marcel Kornacker has posted comments on this change. Change subject: IMPALA-3610: Account for memory used by filters in the coordinator ......................................................................
Patch Set 2: (15 comments) http://gerrit.cloudera.org:8080/#/c/4066/2//COMMIT_MSG Commit Message: Line 25: the output broadcast structure without copying. please add this explanation/invariant (which is very helpful for understanding the code) to the declaration of the affected data structures in the .h file. http://gerrit.cloudera.org:8080/#/c/4066/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: Line 431: filter_mem_tracker_.reset(new MemTracker(-1, -1, "Runtime Filter (Coordinator)", move into c'tor, this is setup that can be done prior to execution. Line 1965: unordered_set<int32_t> target_fragment_instance_idxs; move to where it's used Line 1979: // Check if the filter has already been sent, which could happen in three cases: if turn into bullet points, easier to read Line 1995: --state->pending_count; turn this block into a member fn for state (ApplyUpdate?), that'll make it easier to follow Line 2006: // Discard as one missing update means a correct filter cannot be produced. discard, Line 2007: state->disabled = true; move into discard Line 2013: // TODO: Implement BloomFilter::Or(const ThriftBloomFilter&) if we already have problems with memory spikes, shouldn't this be addressed? there is no need to create a superfluous in-memory filter instead of coding Or to go directly against the thrift structure (do we ever need the non-thrift version?) Line 2026: for (const auto& target: state->targets) { remove auto unless it's an stl iterator Line 2035: // move the payload from the request rather than copy it and take double the memory provide that information for the data structure in the .h file Line 2040: swap(rpc_params->bloom_filter, *filter); why not do this for FilterState in the block above, rather than just the rpc param. and while we're at it, why not also do it for the first oncoming non-broadcast filter? Line 2051: for (const auto& target_idx: target_fragment_instance_idxs) { don't use auto here, this isn't an iterator Line 2054: auto cb = [rpc_params, addr = fragment_inst->impalad_address(), bind was more compact; please revert http://gerrit.cloudera.org:8080/#/c/4066/2/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: Line 195: void Done(); TearDown? done is very generic. Line 399: std::unique_ptr<BloomFilter> bloom_filter; is this why you're including the filter header? move FilterState into .cc instead (and remove include), we need to curtail rampant includes. -- 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: 2 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
