Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6535 )

Change subject: IMPALA-2550: Switch to per-query exec rpc
......................................................................


Patch Set 15:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6535/15/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

http://gerrit.cloudera.org:8080/#/c/6535/15/be/src/runtime/coordinator-backend-state.cc@363
PS15, Line 363:   local_params.__set_bloom_filter(rpc_params->bloom_filter);
> Sorry for commenting on an old CR. I'm reading this part of code and have s
Note that this change only moved the code around (into coordinator.cc), it 
didn't introduce this code. You'll have to go further back in time to see the 
change that introduced that code, and maybe there is insight there.

That said, I agree it doesn't seem like shared_ptr makes sense here (and 
usually, we should avoid use of shared_ptr in impala because it makes reasoning 
about lifetimes difficult).

>From the comment in Coordinator::UpdateFilter() it sounds like there was some 
>idea to parallelize the RPCs, which maybe is where the thought of shared_ptr 
>came in. I'm not sure if the code ever did that, though. Going back further in 
>git history may answer it.

// Make a 'master' copy that will be shared by all concurrent delivery RPC 
attempts.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I20769e420711737b6b385c744cef4851cee3facd
Gerrit-Change-Number: 6535
Gerrit-PatchSet: 15
Gerrit-Owner: Marcel Kornacker <[email protected]>
Gerrit-Reviewer: Dan Hecht <[email protected]>
Gerrit-Reviewer: Henry Robinson <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <[email protected]>
Gerrit-Reviewer: Michael Ho <[email protected]>
Gerrit-Reviewer: Tianyi Wang <[email protected]>
Gerrit-Reviewer: Tim Armstrong <[email protected]>
Gerrit-Comment-Date: Mon, 25 Sep 2017 22:43:20 +0000
Gerrit-HasComments: Yes

Reply via email to