You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2016/10/14 04:14:27 UTC
[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
Sailesh Mukil has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4724
Change subject: KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
......................................................................
KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
If Messenger::Init() fails in a debug build, a CHECK() will happen on
the destruction of the Messenger object.
This patch reorders some code in the MessengerBuilder::Build()
functions to make sure that doesn't happen.
This case was probably never encountered as Messenger::Init()
currently has a very low chance of failing. However, in the future, as
more things may get added on to the function, this issue might show
up more often.
A more detailed explanation is given in the JIRA.
Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
---
M src/kudu/rpc/messenger.cc
1 file changed, 3 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/4724/1
--
To view, visit http://gerrit.cloudera.org:8080/4724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4724
to look at the new patch set (#2).
Change subject: KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
......................................................................
KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
If Messenger::Init() fails in a debug build, a CHECK() will happen on
the destruction of the Messenger object. This happens beacuse, on an
error, the Messenger is not Shutdown().
This patch gets rid of the private function
Messenger::Build(Messenger**) and moves all its logic into the public
function Build(shared_ptr<Messenger>*). On an error, an explicit call
to AllExternalReferencesDropped() is made.
This case was probably never encountered as Messenger::Init()
currently has a very low chance of failing. However, in the future, as
more things may get added on to the function, this issue might show
up more often.
A more detailed explanation is given in the JIRA.
Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
2 files changed, 7 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/4724/2
--
To view, visit http://gerrit.cloudera.org:8080/4724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4724
to look at the new patch set (#3).
Change subject: KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
......................................................................
KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
If Messenger::Init() fails in a debug build, a CHECK() will happen on
the destruction of the Messenger object. This happens beacuse, on an
error, the Messenger is not Shutdown().
This patch gets rid of the private function
Messenger::Build(Messenger**) and moves all its logic into the public
function Build(shared_ptr<Messenger>*). On an error, an explicit call
to AllExternalReferencesDropped() is made.
This case was probably never encountered as Messenger::Init()
currently has a very low chance of failing. However, in the future, as
more things may get added on to the function, this issue might show
up more often.
A more detailed explanation is given in the JIRA.
Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
2 files changed, 7 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/4724/3
--
To view, visit http://gerrit.cloudera.org:8080/4724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
......................................................................
KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
If Messenger::Init() fails in a debug build, a CHECK() will happen on
the destruction of the Messenger object. This happens beacuse, on an
error, the Messenger is not Shutdown().
This patch gets rid of the private function
Messenger::Build(Messenger**) and moves all its logic into the public
function Build(shared_ptr<Messenger>*). On an error, an explicit call
to AllExternalReferencesDropped() is made.
This case was probably never encountered as Messenger::Init()
currently has a very low chance of failing. However, in the future, as
more things may get added on to the function, this issue might show
up more often.
A more detailed explanation is given in the JIRA.
Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Reviewed-on: http://gerrit.cloudera.org:8080/4724
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
2 files changed, 7 insertions(+), 11 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4724/1/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
PS1, Line 103: Status MessengerBuilder::Build(Messenger **msgr) {
: RETURN_NOT_OK(SaslInit(kSaslAppName)); // Initialize SASL library before we start making requests
: gscoped_ptr<Messenger> new_msgr(new Messenger(*this));
: *msgr = new_msgr.release();
: RETURN_NOT_OK((*msgr)->Init());
: return Status::OK();
: }
since this is just a private method called from one place, maybe we should just move all of this into the other Build? With this re-ordering you've done it looks like a buggy function since our usual policy is that if it's a bad status then nothing happens in the out-params. Any reason not to merge the two functions?
--
To view, visit http://gerrit.cloudera.org:8080/4724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4724/1/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
PS1, Line 103: Status MessengerBuilder::Build(Messenger **msgr) {
: RETURN_NOT_OK(SaslInit(kSaslAppName)); // Initialize SASL library before we start making requests
: gscoped_ptr<Messenger> new_msgr(new Messenger(*this));
: *msgr = new_msgr.release();
: RETURN_NOT_OK((*msgr)->Init());
: return Status::OK();
: }
> since this is just a private method called from one place, maybe we should
Yes merging both of them makes sense to me.
But the other Build() will still require the Deleter to be added to it before returning it, on an error. Does that also count as something happening in the out-params?
--
To view, visit http://gerrit.cloudera.org:8080/4724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4724/1/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
PS1, Line 103: Status MessengerBuilder::Build(Messenger **msgr) {
: RETURN_NOT_OK(SaslInit(kSaslAppName)); // Initialize SASL library before we start making requests
: gscoped_ptr<Messenger> new_msgr(new Messenger(*this));
: *msgr = new_msgr.release();
: RETURN_NOT_OK((*msgr)->Init());
: return Status::OK();
: }
> Yes merging both of them makes sense to me.
I think in the case of an error you'd want to just call AllExternalReferencesDropped, or otherwise make sure you don't actually set the out-param on error.
--
To view, visit http://gerrit.cloudera.org:8080/4724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4724/2/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
Line 106: Status build_status = new_msgr->Init();
> warning: redundant get() call on smart pointer [google-readability-redundan
Done
Line 108: new_msgr->AllExternalReferencesDropped();
> warning: redundant get() call on smart pointer [google-readability-redundan
Done
--
To view, visit http://gerrit.cloudera.org:8080/4724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4724/1/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
PS1, Line 103: Status MessengerBuilder::Build(shared_ptr<Messenger> *msgr) {
: RETURN_NOT_OK(SaslInit(kSaslAppName)); // Initialize SASL library before we start making requests
: gscoped_ptr<Messenger> new_msgr(new Messenger(*this));
: Status build_status = new_msgr.get()->Init();
: if (!build_status.ok()) {
: new_msgr.get()->AllExternalReferencesDropped();
:
> I think in the case of an error you'd want to just call AllExternalReferenc
Yes of course, don't know what I was thinking. I've merged both the functions now.
--
To view, visit http://gerrit.cloudera.org:8080/4724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes