impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610
Date Tue, 06 Sep 2016 20:45:16 GMT
Marcel Kornacker has posted comments on this change.

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

Patch Set 2:

File be/src/runtime/

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
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
To unsubscribe, visit

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-HasComments: Yes

View raw message