Marcel Kornacker has posted comments on this change. Change subject: IMPALA-3610: Account for memory used by filters in the coordinator ......................................................................
Patch Set 10: (5 comments) looks good, but i'd still like to see the filter swapping cleaned up. http://gerrit.cloudera.org:8080/#/c/4066/9/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: Line 718: row.push_back(lexical_cast<string>(v.first)); > then why not remove the 'const' in the for? ? Line 2048: state->Disable(filter_mem_tracker_.get()); > this is an iterator, no? you don't need to create refs to iterators Line 2107: // cost. After this point, params.bloom_filter is an empty filter and should not be > that's right, and FilterState has all the information it needs to do this c ? http://gerrit.cloudera.org:8080/#/c/4066/10/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: Line 262: /// A filter may be disabled if an always_true filter update is received, an OOM is hit, "may be" is too vague; "is"? (what else may happen?) Line 332: /// True if the filter is permanently disabled for this query due to being disabled or circular definition ("is disabled due to being disabled"). you already define the semantics of 'disabled' in the class comment, no need to repeat. -- 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: 10 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
