Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20365 )
Change subject: KUDU-3504 Crash master on subprocess death ...................................................................... Patch Set 11: (2 comments) http://gerrit.cloudera.org:8080/#/c/20365/11/src/kudu/subprocess/server.cc File src/kudu/subprocess/server.cc: http://gerrit.cloudera.org:8080/#/c/20365/11/src/kudu/subprocess/server.cc@109 PS11, Line 109: exit_on_failure I think it's important to have a kill switch for this sort of behavior -- there might be some unintended consequences of this change in the behavior, so having a kill switch could help to get out of an unexpected trouble. I'm OK with making the new flag 'true' by default (i.e. have the new behavior by default), but please add a flag that users could set to control whether subprocess server crashes when the subprocess peer exits with non-success code. http://gerrit.cloudera.org:8080/#/c/20365/11/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: http://gerrit.cloudera.org:8080/#/c/20365/11/src/kudu/util/subprocess.cc@280 PS11, Line 280: load() nit: AFAIK, the casting operators in the standard C++ library work with std::atomic as expected, so no load() call is necessary BTW, there are other places in this code that access child_pid_ and state_ without calling load(), so maybe consider either dropping load() call everywhere or add it everywhere if you want to emphasize that those fields are atomics. I'd prefer the former, but I'm OK with the latter as well; but please make that uniform across this code. -- To view, visit http://gerrit.cloudera.org:8080/20365 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec516f3d684f152bd29874b60b810c526ee5a184 Gerrit-Change-Number: 20365 Gerrit-PatchSet: 11 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 18 Aug 2023 00:40:25 +0000 Gerrit-HasComments: Yes
