Ádám Bakai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19613 )
Change subject: Exit external_mini_cluster services when test is stopped ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/19613/2/src/kudu/kserver/kserver.cc File src/kudu/kserver/kserver.cc: http://gerrit.cloudera.org:8080/#/c/19613/2/src/kudu/kserver/kserver.cc@60 PS2, Line 60: TAG_FLAG(exit_if_orphaned, hidden); > I guess this flag should be marked 'unsafe' because kudu servers started no Done http://gerrit.cloudera.org:8080/#/c/19613/2/src/kudu/master/master_runner.cc File src/kudu/master/master_runner.cc: http://gerrit.cloudera.org:8080/#/c/19613/2/src/kudu/master/master_runner.cc@29 PS2, Line 29: #include <unistd.h> > nit: move this up to come before C++ headers, separating from C++ headers w Done http://gerrit.cloudera.org:8080/#/c/19613/2/src/kudu/master/master_runner.cc@467 PS2, Line 467: while (true) { : SleepFor(MonoDelta::FromSeconds(1)); : if (FLAGS_exit_if_orphaned && getppid() == 1) return Status::OK(); : } > Instead of introducing and dancing around globals (like FLAGS_exit_if_orpha I don't see how creating RunMasterServerMiniCluster and RunMasterServerStandalone would help. It's perfectly valid to start kudu master with "kudu master run" command, there is no way at least right now without introducing some mechanism to decide if the master is running in mini cluster or as a stand-alone( with kudu master run). I made a different type of simplification, though, where both runners just calls ServerBase::WaitFinished() (which could be static function right now, but it isn't because I don't see any reason that it couldnt be changed to use some member variables. http://gerrit.cloudera.org:8080/#/c/19613/2/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/19613/2/src/kudu/mini-cluster/external_mini_cluster.cc@1789 PS2, Line 1789: Status ExternalTabletServer::Restart() { > What about Restart()? Should the set of flags for "tserver run" be updated Done -- To view, visit http://gerrit.cloudera.org:8080/19613 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ca88b3b8924dc7dc5c05a0239dfa86db67af255 Gerrit-Change-Number: 19613 Gerrit-PatchSet: 2 Gerrit-Owner: Ádám Bakai <aba...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Chovan <zcho...@cloudera.com> Gerrit-Reviewer: Ádám Bakai <aba...@cloudera.com> Gerrit-Comment-Date: Thu, 23 Mar 2023 14:17:22 +0000 Gerrit-HasComments: Yes
