You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2017/06/12 09:03:27 UTC
[kudu-CR] mini master: remove CHECK from destructor
Hello Andrew Wong, Mike Percy,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7151
to review the following change.
Change subject: mini_master: remove CHECK from destructor
......................................................................
mini_master: remove CHECK from destructor
This CHECK isn't safe; it can fire during test setup before the master has
been shut down. For example, consider this (edited) code snippet from
MiniCluster::StartSingleMaster():
gscoped_ptr<MiniMaster> mini_master(new MiniMaster(...));
RETURN_NOT_OK(mini_master->Start());
RETURN_NOT_OK(mini_master->master()->WaitForSomething(...));
mini_masters_.push_back(shared_ptr<MiniMaster>(mini_master.release()));
MiniCluster's destructor guarantees that the masters are shut down before
being destroyed, but if WaitForSomething() fails, the new master won't be
added to mini_masters_ and thus will be destroyed without first being shut
down at the end of StartSingleMaster().
This is the cause of one source of delete_tablet-itest flakiness.
Change-Id: I742e190f751535d2a59354c3b35cdaf0a340d4ea
---
M src/kudu/master/mini_master.cc
1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/7151/1
--
To view, visit http://gerrit.cloudera.org:8080/7151
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I742e190f751535d2a59354c3b35cdaf0a340d4ea
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] mini cluster: shutdown in destructors
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7151
to look at the new patch set (#2).
Change subject: mini_cluster: shutdown in destructors
......................................................................
mini_cluster: shutdown in destructors
The CHECK in the MiniMaster destructor isn't safe; it can fire during test
setup before the master has been shut down. For example, consider this
(edited) code snippet from MiniCluster::StartSingleMaster():
gscoped_ptr<MiniMaster> mini_master(new MiniMaster(...));
RETURN_NOT_OK(mini_master->Start());
RETURN_NOT_OK(mini_master->master()->WaitForSomething(...));
mini_masters_.push_back(shared_ptr<MiniMaster>(mini_master.release()));
MiniCluster's destructor guarantees that the masters are shut down before
being destroyed, but if WaitForSomething() fails, the new master won't be
added to mini_masters_ and thus will be destroyed without first being shut
down at the end of StartSingleMaster(). To make this safe, let's remove the
CHECK and shut down the master in the destructor. I rolled this out to
MiniTabletServer too, for consistency.
The CHECK (and timeout in StartSingleMaster()) is the cause of one source of
flakiness in delete_tablet-itest.
Change-Id: I742e190f751535d2a59354c3b35cdaf0a340d4ea
---
M src/kudu/integration-tests/mini_cluster.cc
M src/kudu/master/mini_master.cc
M src/kudu/tserver/mini_tablet_server.cc
3 files changed, 4 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/7151/2
--
To view, visit http://gerrit.cloudera.org:8080/7151
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I742e190f751535d2a59354c3b35cdaf0a340d4ea
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] mini cluster: shutdown in destructors
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: mini_cluster: shutdown in destructors
......................................................................
mini_cluster: shutdown in destructors
The CHECK in the MiniMaster destructor isn't safe; it can fire during test
setup before the master has been shut down. For example, consider this
(edited) code snippet from MiniCluster::StartSingleMaster():
gscoped_ptr<MiniMaster> mini_master(new MiniMaster(...));
RETURN_NOT_OK(mini_master->Start());
RETURN_NOT_OK(mini_master->master()->WaitForSomething(...));
mini_masters_.push_back(shared_ptr<MiniMaster>(mini_master.release()));
MiniCluster's destructor guarantees that the masters are shut down before
being destroyed, but if WaitForSomething() fails, the new master won't be
added to mini_masters_ and thus will be destroyed without first being shut
down at the end of StartSingleMaster(). To make this safe, let's remove the
CHECK and shut down the master in the destructor. I rolled this out to
MiniTabletServer too, for consistency.
The CHECK (and timeout in StartSingleMaster()) is the cause of one source of
flakiness in delete_tablet-itest.
Change-Id: I742e190f751535d2a59354c3b35cdaf0a340d4ea
Reviewed-on: http://gerrit.cloudera.org:8080/7151
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/integration-tests/mini_cluster.cc
M src/kudu/master/mini_master.cc
M src/kudu/tserver/mini_tablet_server.cc
3 files changed, 4 insertions(+), 3 deletions(-)
Approvals:
Mike Percy: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7151
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I742e190f751535d2a59354c3b35cdaf0a340d4ea
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] mini master: remove CHECK from destructor
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: mini_master: remove CHECK from destructor
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7151/1/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 51
Would it make more sense to just call Shutdown() instead?
--
To view, visit http://gerrit.cloudera.org:8080/7151
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I742e190f751535d2a59354c3b35cdaf0a340d4ea
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] mini master: remove CHECK from destructor
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: mini_master: remove CHECK from destructor
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7151/1/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 51
> I didn't run dist-test, but this won't actually fix the flakiness, because
As long as threads don't keep running I'm fine with it.
--
To view, visit http://gerrit.cloudera.org:8080/7151
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I742e190f751535d2a59354c3b35cdaf0a340d4ea
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] mini master: remove CHECK from destructor
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: mini_master: remove CHECK from destructor
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7151
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I742e190f751535d2a59354c3b35cdaf0a340d4ea
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No
[kudu-CR] mini cluster: shutdown in destructors
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: mini_cluster: shutdown in destructors
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7151
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I742e190f751535d2a59354c3b35cdaf0a340d4ea
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No
[kudu-CR] mini master: remove CHECK from destructor
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: mini_master: remove CHECK from destructor
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7151/1/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 51
> Would it make more sense to just call Shutdown() instead?
~MiniTabletServer() doesn't, so I didn't do that here either.
--
To view, visit http://gerrit.cloudera.org:8080/7151
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I742e190f751535d2a59354c3b35cdaf0a340d4ea
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] mini master: remove CHECK from destructor
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: mini_master: remove CHECK from destructor
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7151/1/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 51
> Still might worth considering since Shutdown() may be doing different thing
I didn't run dist-test, but this won't actually fix the flakiness, because I think the underlying issue is a timeout in the code snippet in the commit description. I guess I should fix that too.
--
To view, visit http://gerrit.cloudera.org:8080/7151
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I742e190f751535d2a59354c3b35cdaf0a340d4ea
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] mini cluster: shutdown in destructors
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: mini_cluster: shutdown in destructors
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7151/1/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 51
> As long as threads don't keep running I'm fine with it.
When the MiniMaster is deleted, without calling MIniMaster::Shutdown(), it'll just call the destructors of its members, right?
it looks like the d'tor for master_ checks that the its state isn't kRunning, which is only true after the call to Master::Shutdown(), which isn't the case if only the d'tor is called, so I think here we should be calling MiniMaster::Shutdown() or remove the check in Master's d'tor.
--
To view, visit http://gerrit.cloudera.org:8080/7151
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I742e190f751535d2a59354c3b35cdaf0a340d4ea
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] mini master: remove CHECK from destructor
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: mini_master: remove CHECK from destructor
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7151/1/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 51
> ~MiniTabletServer() doesn't, so I didn't do that here either.
Still might worth considering since Shutdown() may be doing different things for tserver and master. Off the top of my head, I'm not sure if they have the same d'tor behavior, but there may be checks along the d'tor in master_, for instance.
Did you run dist-tests to verify the check failures went away (since this is only one source of errors, not sure if that is still the best strategy in verify the removal of flaky code)?
--
To view, visit http://gerrit.cloudera.org:8080/7151
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I742e190f751535d2a59354c3b35cdaf0a340d4ea
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes