impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator
Date Wed, 31 Aug 2016 06:30:08 GMT
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 <henry@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message