Henry Robinson has posted comments on this change. Change subject: IMPALA-3610: Account for memory used by filters in the coordinator ......................................................................
Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/3618/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS2, Line 2077: state->DiscardFilter(filter_mem_tracker_.get()); > Isn't this essentially a no-op, because an always_true_filter will always j it's the latter. One partitioned filter provider might run out of memory and abort its filter construction, but would send a NULL filter to stop the coordinator waiting for it. PS2, Line 2090: state->pending_count == 0 > Wouldn't this always be 0 for a broadcast join filter? Given that we always You're right, good catch. This is always 0 at this point for a broadcast join. Better to track the number received explicitly rather than overloading what pending_count means for different filter types. PS2, Line 2110: for (const auto& target: state->targets) { : // Don't publish the filter to targets that are in the same fragment as the join : // that produced it. : if (target.is_local) continue; : target_fragment_instance_idxs.insert(target.fragment_instance_idxs.begin(), : target.fragment_instance_idxs.end()); : } > Seems like state->targets is read only at this point, because it's set only Are you worried that this is an expensive loop? Otherwise I think it's better to have something that's clearly correct (access shared state under lock) than to have an optimization that is correct, but not obviously so -- To view, visit http://gerrit.cloudera.org:8080/3618 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
