You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2016/09/13 01:14:51 UTC
[kudu-CR] [client] avoid circular deps in time-based flusher
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4395
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/1
--
To view, visit http://gerrit.cloudera.org:8080/4395
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I59825981a600f5882ee476479c2ddf16b495c1f9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [client] avoid circular deps in time-based flusher
......................................................................
Patch Set 1:
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?
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert 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 includes the KuduSession which has a strong reference.
Line 517: _1, weak_messenger, weak_session, false),
std::move both messenger and session.
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?
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client] avoid circular deps in time-based flusher
......................................................................
Patch Set 1:
> (1 comment)
>
> I left you some comments just as you revved PS2. Not sure if you
> saw them.
Thank for the reminder -- I addressed them as well.
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Tested-by: Todd Lipcon <to...@apache.org>
---
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client] avoid circular deps in time-based flusher
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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<rpc::Messenger>& 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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes