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 <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
