Adar Dembo has posted comments on this change. Change subject: [client] avoid circular deps in time-based flusher ......................................................................
Patch Set 1: (4 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)) { > I changed the session to keep a weak reference only. But it should be true Can't it be false if it's called out of ScheduleOnReactor()? Line 517: _1, weak_messenger, weak_session, false), > Good suggestion -- will do. But just to make sure I'm not missing anything Yeah, it doesn't affect correctness. 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); > Using the reference is not crucial, just to be consistent with other places I agree with Dan that it'd be cleanest to pass the messenger and the client in the same way. PS1, Line 161: // The reference to the client's messenger (keeping the reference instead of : // declaring friendship to KuduClient and accessing it via the client_). Should update this comment to explain why it's a weak pointer. Why does it need to be weak, actually? Now we know we'll have weak references from the timer task to the session and manager; isn't that enough? -- 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
