Marcel Kornacker has posted comments on this change. Change subject: IMPALA-3610: Account for memory used by filters in the coordinator ......................................................................
Patch Set 9: (13 comments) http://gerrit.cloudera.org:8080/#/c/4066/9/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: Line 262: /// A filter may be discarded if an always_true filter update is received, an OOM is hit, you use both disabled and discarded to mean the same thing (i think). let's switch to disabled, because discarded sounds like it's not there anymore. rename the function to Disable(). mention that a disabled filter consumes no memory. Line 303: /// Resets bloom_filter_ to NULL, sets pending_count_ to 0, releases any memory don't reference internal state in the comment of a public function (the reader should be able to make use of this class w/o understanding the internal state). you already define what you mean by disabled in the class comment. Line 390: // comprehensive logging of all mem-trackers. add TODO-MT: remove Line 718: // Cast the const away to read non-const member functions. No modification is done. why do you need non-const access? Line 726: for (const auto& target: *state.targets()) { no auto Line 2048: for (auto& filter: filter_routing_table_) { why make this a ref? Line 2086: DCHECK_GT(state->pending_count(), 0); from l2086 to l2090 can also go into ApplyUpdate() Line 2099: // If the filter was discarded, we should send out an always_true filter immediately. this comment doesn't describe the following statement Line 2105: // No more filters are pending on this filter ID. Create a distribution payload and "no more updates are..." Line 2107: state->set_completion_time(query_events_->ElapsedTime()); move into ApplyUpdate Line 2119: if (state->desc().is_broadcast_join) { also swap in the filter in ApplyUpdate for broadcast joins, then you don't have to differentiate here. also, it makes the filter state more regular and easier to comprehend. Line 2183: pending_count_ = 0L; does the pending count still matter at this point (and if so, why?) http://gerrit.cloudera.org:8080/#/c/4066/9/be/src/util/bloom-filter-test.cc File be/src/util/bloom-filter-test.cc: Line 277: BloomFilter::ToThrift(&bf3, &bf3_thrift); trailing spaces -- 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: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
