Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9015 )
Change subject: KUDU-2208 Add unit test to detect Subprocess Interruption Handling failure and patch to fix the bug ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/os-util.h File src/kudu/util/os-util.h: http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/os-util.h@33 PS5, Line 33: static_assert(std::is_signed<decltype(err)>::value, \ need to #include <type_traits> in this header so that we get access to std::is_signed. Also whatever header has EINTR in it (not sure which one) http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/subprocess-test.cc File src/kudu/util/subprocess-test.cc: http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/subprocess-test.cc@28 PS5, Line 28: #include <pthread.h> per Alexey's comment please separate out the C includes from the C++ ones https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/subprocess-test.cc@312 PS5, Line 312: t_started = true; still think we should add a short sleep here so that we ensure that the signal-sending starts before we call p.Start http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/subprocess-test.cc@313 PS5, Line 313: ASSERT_OK(p.Start()); nit: we have to use CHECK_OK here because gtest isn't thread-safe to run assertions from non-main threads http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/subprocess-test.cc@332 PS5, Line 332: // Add delay to avoid user defined signal 2 error but the "user defined signal 2 error" is the bug we're trying to provoke here. So if we are still seeing a crash due to it, then that means we haven't fully fixed the bug, right? -- To view, visit http://gerrit.cloudera.org:8080/9015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 Gerrit-Change-Number: 9015 Gerrit-PatchSet: 5 Gerrit-Owner: Jeffrey F. Lukman <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 12 Jan 2018 20:56:24 +0000 Gerrit-HasComments: Yes
