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 2023/04/15 21:04:40 UTC
[kudu-CR] Exit external mini cluster services when test is stopped
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 <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
Gerrit-Comment-Date: Sat, 15 Apr 2023 21:04:40 +0000
Gerrit-HasComments: Yes