Alexey Serbin has posted comments on this change.

Change subject: [client] avoid circular deps in time-based flusher
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4395/1/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:

Line 514:   if (PREDICT_TRUE(messenger)) {
> This can't ever be false, right?  I think the callstack of this method incl
I changed the session to keep a weak reference only.  But it should be true, 
otherwise this code is not supposed to be called, right.  However, for sanity 
it would like to keep this, if there aren't objections.


Line 517:                     _1, weak_messenger, weak_session, false),
> std::move both messenger and session.
Good suggestion -- will do.  But just to make sure I'm not missing anything 
here: adding std::move is not crucial here, right?


http://gerrit.cloudera.org:8080/#/c/4395/1/src/kudu/client/session-internal.h
File src/kudu/client/session-internal.h:

Line 58:                 const sp::weak_ptr<rpc::Messenger>& messenger);
> I think this should be std::, and why use a reference?
Using the reference is not crucial, just to be consistent with other places 
where we don't want to update reference counter.  But I agree updating the 
counter while passing the parameter would be safer.

Right, thanks -- and that's why there was an error when building it at Linux, 
not OS X.

Will change.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I59825981a600f5882ee476479c2ddf16b495c1f9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to