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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
