[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4053: Address follow up comments for IMPALA-3610
..


Patch Set 3:

There was quite some disagreement with the approach. The point of this patch 
was to retry broadcast filter updates if they hit an OOM and another case where 
we try to save memory.

I think we can abandon this for now, since it's not a pain point and revisit as 
a part of IMPALA-3825.

-- 
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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610

[Sailesh Mukil (Code Review)]

Sailesh Mukil has abandoned this change.

Change subject: IMPALA-4053: Address follow up comments for IMPALA-3610
..


Abandoned

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I7499558cc9d3622a9f3b68904aa5fe92a113f579
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4053: Address follow up comments for IMPALA-3610
..


Patch Set 3:

Is this still relevant? Sailesh, what should we do with this change?

-- 
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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4053: Address follow up comments for IMPALA-3610
..


Patch Set 2:

(1 comment)

let's put this on hold until mostafa verifies that the general approach fixes 
the observed memory issue.

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

Line 2090: state->AssignOutgoingFilter(params, filter_mem_tracker_.get(), 
rpc_params.get());
> We didn't need to TryConsume() memory for a broadcast filter update as the 
i think you're over-finessing this. if a single 8mb filter makes the query 
exceed a limit, things are precariously tight anyway, and chances are the query 
will not finish. trying to optimize for the case where the first filter is over 
the limit but maybe one of the subsequent filters isn't won't change the 
picture. it is better to keep the code simple.


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610

[Sailesh Mukil (Code Review)]

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* targets() { return _; }
> 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 target_fragment_instance_idxs;
> remove blank line
Done


Line 2077: lock_guard 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 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes