You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2016/09/02 23:28:52 UTC
[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610
Sailesh Mukil has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4306
Change subject: IMPALA-4053: Address follow up comments for IMPALA-3610
......................................................................
IMPALA-4053: Address follow up comments for IMPALA-3610
This is a follow up to the 'Account memory for global runtime filter
aggregation patch'.
This follow up contains:
* Broadcast filter updates don't TryConsume() memory anymore and are
sent immediately. (So the first one will always succeed, subsequent
ones will be ignored)
* 'disable' filter happens only for error cases. They are discarded
otherwise.
* Some style changes.
Change-Id: I7499558cc9d3622a9f3b68904aa5fe92a113f579
---
M be/src/runtime/coordinator.cc
1 file changed, 62 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/4306/1
--
To view, visit http://gerrit.cloudera.org:8080/4306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7499558cc9d3622a9f3b68904aa5fe92a113f579
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4053: Address follow up comments for IMPALA-3610
......................................................................
Patch Set 2:
(1 comment)
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());
> i think you're over-finessing this. if a single 8mb filter makes the query
That may be true - but just a reminder that filters may (theoretically) be up to 512MB big, so we're not just concerned with the situation where the query is teetering right on the edge of over-consumption.
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4053: Address follow up comments for IMPALA-3610
......................................................................
Patch Set 2:
(9 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?
Line 264: /// it is disabled.
more interesting: how is the discarded or disabled state recorded in this struct?
Line 277: vector<FilterTarget>* targets() { return &targets_; }
FilterTarget isn't referenced in the .h file, also move it here
Line 290: /// Discards a filter as we will use it no more. A discarded filter consumes no memory.
don't comment on the intention of the caller. describe the externally visible behavior of this call instead.
Line 295: MemTracker* filter_mem_tracker, TPublishFilterParams* rpc_params);
describe externally visible behavior and role of parameters. just looking at the signature and the comment doesn't tell me what's going on.
Line 2075:
remove blank line
Line 2077:
and this. this section of code deals with applying updates, so it's a good idea to group it
Line 2082: for (FilterTarget target: *state->targets()) {
const FilterTarget&
since you're not updating the targets here, you can also add another accessor
const vector<FilterTarget>& targets() const { return ...}
Line 2090: state->AssignOutgoingFilter(params, filter_mem_tracker_.get(), rpc_params.get());
i find the control flow harder to follow than before, and the code has more special cases now. what was the reason for 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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@gmail.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
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 <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#2).
Change subject: IMPALA-4053: Address follow up comments for IMPALA-3610
......................................................................
IMPALA-4053: Address follow up comments for IMPALA-3610
This is a follow up to the 'Account memory for global runtime filter
aggregation patch'.
This follow up contains:
* Broadcast filter updates don't TryConsume() memory anymore and are
sent immediately. (So the first one will always succeed, subsequent
ones will be ignored)
* 'disable' filter happens only for error cases. They are discarded
otherwise.
* Some style changes.
Change-Id: I7499558cc9d3622a9f3b68904aa5fe92a113f579
---
M be/src/runtime/coordinator.cc
1 file changed, 62 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/4306/2
--
To view, visit http://gerrit.cloudera.org:8080/4306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7499558cc9d3622a9f3b68904aa5fe92a113f579
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
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 <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@gmail.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
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 <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@gmail.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-4053: Address follow up comments for IMPALA-3610
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/4306/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
Line 304: // Index into fragment_instance_states_ for source fragment instances.
This does not merely look like an index. Can you comment on why we're using a set here?
Line 733: row.push_back(!state.disabled() ? "true" : "false");
this line would need changing if we move pending_count into disabled()
PS2, Line 2069: (state->disabled() || state->pending_count() == 0)
These two mostly occur together. Maybe move the pending_count check into disabled()? Or make a new method ProcessingFinished() or similar and combine both there.
PS2, Line 2078: && !state->disabled()
Is it correct to check for !state->disabled() here? Can it happen that between L2069 and here the state get's disabled?
PS2, Line 2092: Filter is complete, and can be released.
How about "Filter has been processed and distributed completely, and can be discarded." ?
PS2, Line 2147: if (pending_count_ == 0 || disabled_) {
: completion_time_ = coord->query_events_->ElapsedTime();
: }
Could this go into Discard()?
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#3).
Change subject: IMPALA-4053: Address follow up comments for IMPALA-3610
......................................................................
IMPALA-4053: Address follow up comments for IMPALA-3610
This is a follow up to the 'Account memory for global runtime filter
aggregation patch'.
This follow up contains:
* If a broadcast filter update hits an OOM, subsequent updates will
retry as they may come in at a time of lower memory pressure.
* 'disable' filter happens only for error cases. They are discarded
otherwise.
* Some style changes.
Change-Id: I7499558cc9d3622a9f3b68904aa5fe92a113f579
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
2 files changed, 92 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/4306/3
--
To view, visit http://gerrit.cloudera.org:8080/4306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7499558cc9d3622a9f3b68904aa5fe92a113f579
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
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 <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes