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 <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 09 Oct 2017 19:01:30 +0000 Gerrit-HasComments: Yes
