Alexey Serbin 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: (2 comments) 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@467 PS2, Line 467: while (true) { : SleepFor(MonoDelta::FromSeconds(1)); : if (FLAGS_exit_if_orphaned && getppid() == 1) return Status::OK(); : } > 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). There is a simple criterion right now to decide if the master (or tablet server) is running as a part of external mini-cluster used for tests: those processes are started as a part of the external mini-cluster via 'kudu master run' and 'kudu tserver run' correspondingly. That's done using MasterRun() in tool_action_master.cc and TServerRun() in tool_action_tserver.cc. The idea is to add an appropriate parameter for tserver::RunTabletServer() and master::RunMasterServer(). As far as I can see, no other tests run those processes otherwise since changelist 2e580fe863b086cf16fd9d285dd56ba817f64cd3. With that, I don't see any reason to introduce a new flag here to "exit external_mini_cluster services when test is stopped". As for ServerBase::WaitFinished(): if a member function doesn't use any member fields, then it should be made static. Whoever changes it later to use any member fields, can update the code accordingly. At least, that's how it should be handled based on the YAGNI design principle: https://www.martinfowler.com/bliki/Yagni.html Does it make sense to you? http://gerrit.cloudera.org:8080/#/c/19613/4/src/kudu/server/server_base.h File src/kudu/server/server_base.h: http://gerrit.cloudera.org:8080/#/c/19613/4/src/kudu/server/server_base.h@145 PS4, Line 145: ected: Wait indefinitely for what? Also, why to return Status? If it has any non-trivial semantic, please document the convention on the return status. Otherwise, change the signature to return 'void'? -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Ádám Bakai <[email protected]> Gerrit-Comment-Date: Sat, 15 Apr 2023 21:04:40 +0000 Gerrit-HasComments: Yes
