Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10435 )
Change subject: KUDU-2427: retry more system calls on EINTR ...................................................................... Patch Set 6: (17 comments) It seems we also have some calls with the EINTR errno semantic in src/kudu/tools. Does it make sense to wrap those with RETRY_ON_EINTR() as well? E.g., tools/tool_action_test.cc: PCHECK(dup2(STDERR_FILENO, STDOUT_FILENO) == STDOUT_FILENO); tools/tool_action_common.cc: close(read_fd_); tools/tool_action_common.cc: close(write_fd_); http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/consensus/log_index.cc File src/kudu/consensus/log_index.cc: http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/consensus/log_index.cc@116 PS6, Line 116: RETRY_ON_EINTR(ret, close(fd_)); It's highly unlikely, but is it worth logging about an error? http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/gutil/sysinfo.cc File src/kudu/gutil/sysinfo.cc: http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/gutil/sysinfo.cc@301 PS6, Line 301: RETRY_ON_EINTR(ret, close(fd)); Is it worth logging about a failure (if it not EINTR, of course)? http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/rpc/negotiation.cc File src/kudu/rpc/negotiation.cc: http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/rpc/negotiation.cc@119 PS6, Line 119: RETRY_ON_EINTR(ready, ppoll(&poll_fd, 1, &ts, nullptr)); : #else : RETRY_ON_EINTR(ready, poll(&poll_fd, 1, remaining.ToMilliseconds())); This update changes the logic a bit so the deadline is no longer applied. Is it OK? http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/env_posix.cc@560 PS6, Line 560: fclose Wrap into RETRY_ON_EINTR() and log in case of other error? http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/env_posix.cc@603 PS6, Line 603: close(fd_); Wrap into RETRY_ON_EINTR() and log in case of other error? http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/env_posix.cc@1174 PS6, Line 1174: closedir(d); Wrap into RETRY_ON_EINTR() and log in case of other error? http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/env_posix.cc@1380 PS6, Line 1380: close(fd) Wrap into RETRY_ON_EINTR() here as well? http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/env_posix.cc@1392 PS6, Line 1392: PosixFileLock* my_lock = reinterpret_cast<PosixFileLock*>(lock); nit: maybe, wrap this into unique_ptr<> by the way? http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/env_posix.cc@1397 PS6, Line 1397: close(my_lock->fd_); Wrap into RETRY_ON_EINTR() and log if case of other error? http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/env_posix.cc@1737 PS6, Line 1737: fts_close(fts) nit: not a part of this changelist, but just curious, could this ever fail, and if yes, whether we need any handling for that. http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/semaphore.cc File src/kudu/util/semaphore.cc: http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/semaphore.cc@a66 PS6, Line 66: I hope that was not the desired behavior to return from this method on EINTR error. http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/subprocess.cc@384 PS6, Line 384: == STDIN_FILENO typo? http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/subprocess.cc@385 PS6, Line 385: dup2_ret) dup2_ret == STDIN_FILENO ? http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/subprocess.cc@394 PS6, Line 394: == STDOUT_FILENO ditto http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/subprocess.cc@395 PS6, Line 395: dup2_ret ditto http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/subprocess.cc@411 PS6, Line 411: == STDERR_FILENO ditto http://gerrit.cloudera.org:8080/#/c/10435/6/src/kudu/util/subprocess.cc@412 PS6, Line 412: dup2_ret ditto -- To view, visit http://gerrit.cloudera.org:8080/10435 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cce03c4e1b2be32c1910382737526082fc99966 Gerrit-Change-Number: 10435 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 22 May 2018 22:19:47 +0000 Gerrit-HasComments: Yes
