Jeffrey F. Lukman has posted comments on this change. ( http://gerrit.cloudera.org:8080/9011 )
Change subject: Kudu-2208 Avoid system call interruption in Subprocess ...................................................................... Patch Set 1: (12 comments) http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc File src/kudu/util/subprocess-test.cc: http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@301 PS1, Line 301: thd > nit: either abbreviate to just 't' or 'thr' would be more in line with what Done http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@302 PS1, Line 302: 0.5 > nit: why not just '1'? Using non-integer numbers for /bin/sleep is conside Done http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@303 PS1, Line 303: subprocessThread > nit: we use snake_case style in C++ Done http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@305 PS1, Line 305: ASSERT_OK(p.Start()); > I think adding a short sleep here (eg 50ms) is a good idea to ensure that t Done http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@310 PS1, Line 310: // create signal > you aren't creating a signal here, but rather setting up a signal handler Done http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@317 PS1, Line 317: // send at most 10 kill signals to subprocess > why at most 10? Why not loop until we see that the subprocess has exited? I Done http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@319 PS1, Line 319: int signalCounter = 0; : bool hasError = false; : > nit: snake_case Done http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@322 PS1, Line 322: if (signalCounter > 10 || hasError) break; > why not just put this into the while condition above? Done http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@325 PS1, Line 325: hasError = pthread_kill(thd, SIGUSR2) == 0; > do we expect any errors from this? if not, could we just ASSERT this? Done http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@330 PS1, Line 330: SleepFor(MonoDelta::FromMilliseconds(10)); > why sleep in between? if our goal is to provoke races we want to send as ma I added sleep time here to avoid undefined signal 2 exception. http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess.cc@67 PS1, Line 67: // Retry on EINTR for functions like read() that return -1 on error. > can we move this function to a utility header since it's used in a few plac Done http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess.cc@69 PS1, Line 69: == true > nit: can we drop this extra? Done -- To view, visit http://gerrit.cloudera.org:8080/9011 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I66ab2ec391eced5ea1c37d4294d151fe03c7d586 Gerrit-Change-Number: 9011 Gerrit-PatchSet: 1 Gerrit-Owner: Jeffrey F. Lukman <jeffreyfluk...@gmail.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Fri, 12 Jan 2018 19:30:59 +0000 Gerrit-HasComments: Yes
