impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5789: Add always false flag in bloom filter
Date Mon, 09 Oct 2017 19:01:30 GMT
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8170 )

Change subject: IMPALA-5789: Add always_false flag in bloom filter
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8170/2/be/src/runtime/coordinator-filter-state.h
File be/src/runtime/coordinator-filter-state.h:

http://gerrit.cloudera.org:8080/#/c/8170/2/be/src/runtime/coordinator-filter-state.h@57
PS2, Line 57: class Coordinator::FilterState {
Could you add a comment up here explaining the different meanings of 'always_true' and 'always_false'
?

To me it seems like 'always_false' means either that the FilterState does not hold a bloom
filter yet, or that the bloom filter is always false.

And 'always_true' could mean that the filter is disabled or that the filter it holds is always
true.

Also, do these meanings of 'always_true' and 'always_false' hold true in other parts of the
code as well? i.e. in parts that are not the coordinator?

If it were up to me, I would hold different states for 'disabled_' and 'is_inited_' instead
of trying to derive those meanings implicitly, to make the code easier to read. But that's
up for debate, and we can reach a consensus based on what others say.


http://gerrit.cloudera.org:8080/#/c/8170/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

http://gerrit.cloudera.org:8080/#/c/8170/2/be/src/runtime/coordinator.cc@1121
PS2, Line 1121:     swap(rpc_params.bloom_filter, aggregated_filter);
What are the possible states after this swap() ? Since we're not explicitly setting the disabled
case anymore.

Eg: DCHECK(rpc_params.bloom_filter.directory.size() > 0 || rpc_params.bloom_filter.always_true
== true) ?


http://gerrit.cloudera.org:8080/#/c/8170/2/be/src/runtime/coordinator.cc@1123
PS2, Line 1123: size()
Since you changed the mem tracking from size() to capacity() in L1120 and L1181, shouldn't
this be capacity() too now?

Else, we risk releasing the capacity() twice, if the swap() doesn't assign a 0 capacity string
to the aggregated filter.



-- 
To view, visit http://gerrit.cloudera.org:8080/8170
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If680240a3cd4583fc97c3192177d86d9567c4f8d
Gerrit-Change-Number: 8170
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang <twang@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <twang@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Oct 2017 19:01:30 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message