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: (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 not from 'test mini_cluster' CLI would exit right away. Marking just 'unsafe' is enough because unsafe flags are automatically hidden. 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 with an empty line (you can see how it's done in other files containing unistd.h header). 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_orphaned), maybe add a parameter for RunMasterServer()? Even better, just separate everything from RunMasterServer() except this 'while' loop for into a function, and then use it in two new functions: RunMasterServerMiniCluster() and RunMasterServerStandalone(), calling appropriate functions from master_main.cc and tool_action_master.cc? Use similar approach for RunTabletServer() 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 as well? -- 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: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Sat, 18 Mar 2023 03:03:47 +0000 Gerrit-HasComments: Yes
