[kudu-CR] [client] avoid circular deps in time-based flusher

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

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


[client] avoid circular deps in time-based flusher

The boost::bind() makes cast of parameters during the call,
not during creation of the functor object:
  
http://stackoverflow.com/questions/11255144/why-does-boostbind-store-arguments-of-the-type-passed-in-rather-than-of-the-ty

So, it's necessary to pass weak pointers to the background auto-flush
task to avoid circular dependencies between client::KuduSession::Data
and rpc::Messenger.  Besides, it does not make much sense to store
shared reference to messenger in KuduSession::Data since it's always
passed as a weak reference and then promoting to a shared one during
the call in all usage scenarios.

Thanks to Adar and Todd spotting the usual suspect there.

This is a follow-up for 93be1310d227cf05025864654ca3f6713c2ddc2c.

Change-Id: I59825981a600f5882ee476479c2ddf16b495c1f9
Reviewed-on: http://gerrit.cloudera.org:8080/4395
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
Tested-by: Todd Lipcon 
---
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
2 files changed, 15 insertions(+), 15 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Todd Lipcon: Verified
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I59825981a600f5882ee476479c2ddf16b495c1f9
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [client] avoid circular deps in time-based flusher

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

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


Patch Set 3: Verified+1

I managed to write a test which reliably leaks without this patch:
https://gist.github.com/be6e56af8f40803315aa2c43fbd6c9c6

So, I think this is at least an improvement. Let's pull it into 1.0.0rc0 and do 
some focused client stress testing during the voting period.

-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [client] avoid circular deps in time-based flusher

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 3: Code-Review+2

-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [client] avoid circular deps in time-based flusher

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4395

to look at the new patch set (#3).

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

[client] avoid circular deps in time-based flusher

The boost::bind() makes cast of parameters during the call,
not during creation of the functor object:
  
http://stackoverflow.com/questions/11255144/why-does-boostbind-store-arguments-of-the-type-passed-in-rather-than-of-the-ty

So, it's necessary to pass weak pointers to the background auto-flush
task to avoid circular dependencies between client::KuduSession::Data
and rpc::Messenger.  Besides, it does not make much sense to store
shared reference to messenger in KuduSession::Data since it's always
passed as a weak reference and then promoting to a shared one during
the call in all usage scenarios.

Thanks to Adar and Todd spotting the usual suspect there.

This is a follow-up for 93be1310d227cf05025864654ca3f6713c2ddc2c.

Change-Id: I59825981a600f5882ee476479c2ddf16b495c1f9
---
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
2 files changed, 15 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/4395/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4395
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59825981a600f5882ee476479c2ddf16b495c1f9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [client] avoid circular deps in time-based flusher

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

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


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/3402/

-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [client] avoid circular deps in time-based flusher

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

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


Patch Set 2:

(1 comment)

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

PS2, Line 43: std::
> Nit: don't need std prefix here (if you add using).
Good observation.  However, there is also sp::weak_ptr here for KuduSession, so 
I would like to keep this prefix.


-- 
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [client] avoid circular deps in time-based flusher

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 1:

(1 comment)

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

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_).
> Will add some info into the comment, OK.
Yeah, that sounds reasonable.


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

[kudu-CR] [client] avoid circular deps in time-based flusher

[Alexey Serbin (Code Review)]

Alexey Serbin 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)) {
> Can't it be false if it's called out of ScheduleOnReactor()?
Ah, right observation.  And it will get at this point only in case of 
AUTO_FLUSH_BACKGROUND mode, btw.


Line 517: _1, weak_messenger, weak_session, false),
> Yeah, it doesn't affect correctness.
ok, thanks!


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& messenger);
> I agree with Dan that it'd be cleanest to pass the messenger and the client
Done


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. 
Will add some info into the comment, OK.

I have the following reasoning: we pass it around as a weak reference anyway, 
and then convert into a shared one.  Why would we store it as shared?


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

[kudu-CR] [client] avoid circular deps in time-based flusher

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

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


Patch Set 2:

> How sure are we that this won't negatively affect the
 > non-auto-flush code path? i.e is this risky to pull into 1.0.0 so
 > late in the game?

The auto-flush background task should not be run in any other mode except for 
AUTO_FLUSH_BACKGROUND, so that code should not be executed in other modes.  
That's why I think it's safe to pull into 1.0.0.

-- 
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [client] avoid circular deps in time-based flusher

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 2:

(1 comment)

I left you some comments just as you revved PS2. Not sure if you saw them.

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

PS2, Line 43: std::
Nit: don't need std prefix here (if you add using).


-- 
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [client] avoid circular deps in time-based flusher

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

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


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/3396/

-- 
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [client] avoid circular deps in time-based flusher

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4395

to look at the new patch set (#2).

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

[client] avoid circular deps in time-based flusher

The boost::bind() makes cast of parameters during the call,
not during creation of the functor object:
  
http://stackoverflow.com/questions/11255144/why-does-boostbind-store-arguments-of-the-type-passed-in-rather-than-of-the-ty

So, it's necessary to pass weak pointers to the background auto-flush
task to avoid circular dependencies between client::KuduSession::Data
and rpc::Messenger.  Besides, it does not make much sense to store
shared reference to messenger in KuduSession::Data since it's always
passed as a weak reference and then promoting to a shared one during
the call in all usage scenarios.

Thanks to Adar and Todd spotting the usual suspect there.

This is a follow-up for 93be1310d227cf05025864654ca3f6713c2ddc2c.

Change-Id: I59825981a600f5882ee476479c2ddf16b495c1f9
---
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
2 files changed, 5 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/4395/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4395
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59825981a600f5882ee476479c2ddf16b495c1f9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [client] avoid circular deps in time-based flusher

[Adar Dembo (Code Review)]

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

[kudu-CR] [client] avoid circular deps in time-based flusher

[Alexey Serbin (Code Review)]

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

[kudu-CR] [client] avoid circular deps in time-based flusher

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

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


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/3389/

-- 
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 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No