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 2020/02/21 03:01:27 UTC

[kudu-CR] [master] call Shutdown() in destructor

Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15261


Change subject: [master] call Shutdown() in destructor
......................................................................

[master] call Shutdown() in destructor

Master was reported crashing in tests with stack like below when
catalog manager failed to initialize:

  F0220 11:54:37.240003  1700 master.cc:128] Check failed: kRunning != state_ (2 vs. 2)
  *** Check failure stack trace: ***
      @     0x7ff77ba68202  google::LogMessage::Flush()
      @     0x7ff77ba6c93b  google::LogMessageFatal::~LogMessageFatal()
      @     0x7ff7876ed9ac  kudu::master::Master::~Master()
      @     0x7ff78770f797  kudu::master::RunMasterServer()
      @           0x4dd00f  kudu::master::MasterMain()

I took a quick look into the code and if my understanding is correct,
nothing prevents us calling Shutdown() in destructor, like TabletServer
does.

This patch updates Master's destructor to call Shutdown() to avoid
unneeded crashes like above and unify the behavior of Master and
TabletServer objects.

Change-Id: Icc3c5d0b183175ec6a5f288a30fb8a24bd983962
---
M src/kudu/master/master.cc
1 file changed, 2 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/15261/1
-- 
To view, visit http://gerrit.cloudera.org:8080/15261
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icc3c5d0b183175ec6a5f288a30fb8a24bd983962
Gerrit-Change-Number: 15261
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] [master] call Shutdown() in destructor

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15261 )

Change subject: [master] call Shutdown() in destructor
......................................................................

[master] call Shutdown() in destructor

Master was reported crashing in tests with stack like below when
catalog manager failed to initialize:

  F0220 11:54:37.240003  1700 master.cc:128] Check failed: kRunning != state_ (2 vs. 2)
  *** Check failure stack trace: ***
      @     0x7ff77ba68202  google::LogMessage::Flush()
      @     0x7ff77ba6c93b  google::LogMessageFatal::~LogMessageFatal()
      @     0x7ff7876ed9ac  kudu::master::Master::~Master()
      @     0x7ff78770f797  kudu::master::RunMasterServer()
      @           0x4dd00f  kudu::master::MasterMain()

I took a quick look into the code and if my understanding is correct,
nothing prevents us calling Shutdown() in destructor, like TabletServer
does.

This patch updates Master's destructor to call Shutdown() to avoid
unneeded crashes like above and unify the behavior of Master and
TabletServer objects.

I also did cosmetic updates to the code to squelch cland-tidy warnings.

Change-Id: Icc3c5d0b183175ec6a5f288a30fb8a24bd983962
Reviewed-on: http://gerrit.cloudera.org:8080/15261
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/kserver/kserver.h
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/tablet_server.cc
M src/kudu/tserver/tablet_server.h
7 files changed, 89 insertions(+), 69 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Icc3c5d0b183175ec6a5f288a30fb8a24bd983962
Gerrit-Change-Number: 15261
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>

[kudu-CR] [master] call Shutdown() in destructor

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, Volodymyr Verovkin, 

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

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

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

Change subject: [master] call Shutdown() in destructor
......................................................................

[master] call Shutdown() in destructor

Master was reported crashing in tests with stack like below when
catalog manager failed to initialize:

  F0220 11:54:37.240003  1700 master.cc:128] Check failed: kRunning != state_ (2 vs. 2)
  *** Check failure stack trace: ***
      @     0x7ff77ba68202  google::LogMessage::Flush()
      @     0x7ff77ba6c93b  google::LogMessageFatal::~LogMessageFatal()
      @     0x7ff7876ed9ac  kudu::master::Master::~Master()
      @     0x7ff78770f797  kudu::master::RunMasterServer()
      @           0x4dd00f  kudu::master::MasterMain()

I took a quick look into the code and if my understanding is correct,
nothing prevents us calling Shutdown() in destructor, like TabletServer
does.

This patch updates Master's destructor to call Shutdown() to avoid
unneeded crashes like above and unify the behavior of Master and
TabletServer objects.

I also did cosmetic updates to the code to squelch cland-tidy warnings.

Change-Id: Icc3c5d0b183175ec6a5f288a30fb8a24bd983962
---
M src/kudu/kserver/kserver.h
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/tablet_server.cc
M src/kudu/tserver/tablet_server.h
7 files changed, 89 insertions(+), 69 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icc3c5d0b183175ec6a5f288a30fb8a24bd983962
Gerrit-Change-Number: 15261
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>

[kudu-CR] [master] call Shutdown() in destructor

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15261 )

Change subject: [master] call Shutdown() in destructor
......................................................................


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc3c5d0b183175ec6a5f288a30fb8a24bd983962
Gerrit-Change-Number: 15261
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 21 Feb 2020 04:37:33 +0000
Gerrit-HasComments: No