Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4053: Address follow up comments for IMPALA-3610 ......................................................................
Patch Set 3: (15 comments) http://gerrit.cloudera.org:8080/#/c/4306/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: Line 374 > what's the rationale for leaving this here? The query_mem_tracker_ is accessed by the QueryExecState even after TearDown() is called to release it in QueryExecState::ClearResultsCache() Line 264: is_local = tFilterTarget.is_local_target; > more interesting: how is the discarded or disabled state recorded in this s There is no specific state to denote that a filter has been discarded. It happens as a consequence of other things, i.e. query completion, agg completion or being disabled. I've updated how disabled state is recorded in this struct. Line 277: /// it is disabled. > FilterTarget isn't referenced in the .h file, also move it here Done Line 290: vector<FilterTarget>* targets() { return &targets_; } > don't comment on the intention of the caller. describe the externally visib Done Line 295: const TRuntimeFilterDesc& desc() const { return desc_; } > describe externally visible behavior and role of parameters. just looking a In the new patch the only externally available behavior is the mem tracker being updated and the rpc_params being set. Line 304: /// Releases tracked memory and frees 'bloom_filter'. A discarded filter consumes no > This does not merely look like an index. Can you comment on why we're using As opposed to? These are indices to fragment_instance_states_. Line 733: partition_filter.push_back(target.is_bound_by_partition_columns ? "true" : "false"); > this line would need changing if we move pending_count into disabled() See comment below. PS2, Line 2069: < "Filter received before routing table complete"; > These two mostly occur together. Maybe move the pending_count check into di They both mean different things, so moving the pending_count() check into disabled() would hamper readability (even though they currently could functionally be merged). Line 2075: unordered_set<int32_t> target_fragment_instance_idxs; > remove blank line Done Line 2077: lock_guard<SpinLock> l(filter_lock_); > and this. this section of code deals with applying updates, so it's a good Done PS2, Line 2078: t = filter_routing_tab > Is it correct to check for !state->disabled() here? Can it happen that betw Yes, it can get disabled in ApplyUpdate() Line 2082: } > const FilterTarget& Done Line 2090: // filters that can't affect the aggregated global filter. > i find the control flow harder to follow than before, and the code has more We didn't need to TryConsume() memory for a broadcast filter update as the input filter can just be passed on as the output filter. That's the reason for the extra special casing. However, even if I do change it back to how it was originally, we need to add special cases elsewhere to take care of the following case: We don't want to disable the filter if the first update for a broadcast filter hits OOM in ApplyUpdate(), as subsequent updates may arrive at times of lower memory pressure. This requires changing how we track pending_counts for broadcast filters, adding a couple of special cases in ApplyUpdate() and we don't get the benefit of avoiding using extra memory. I've made the change describing the paragraph above, so you can see how it looks and it'll be easier to decide on what to do about this special casing. PS2, Line 2092: * if the filter could not be allocated > How about "Filter has been processed and distributed completely, and can be The distribution happens after this, so I've left that part out. Changed the rest of it. PS2, Line 2147: // If it's a broadcast filter, future updates may arrive at a time of lower memory : // pressure, so do not disable. : > Could this go into Discard()? It could but it would make the readability a little harder. This is in a way a matching call to L2111. -- To view, visit http://gerrit.cloudera.org:8080/4306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7499558cc9d3622a9f3b68904aa5fe92a113f579 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-HasComments: Yes
