[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9015 ) Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. KUDU-2208 Add RETRY_ON_EINTR() to Subprocess This patch submits a unit test that create a Subprocess thread and while it is starting and waiting, another thread sends kill signals to the Subprocess thread. Since the pthread_kill() signal is sent asynchronously, sometimes the pthread_kill() raises error signal 3. In that condition, the unit test logs the incident as an INFO. Prior to this bug fix patch, the Subprocess throws an exception because it cannot handle the kill signal in Wait() state. To fix the bug, we add RETRY_ON_EINTR() inside Subprocess::DoWait() function. Furthermore, this patch also moves the RETRY_ON_EINTR() function that occurs in many places across the code to os-util.h and it adds alarm reset in the end of TestReadFromStdoutAndStderr in Subprocess-test. Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 Reviewed-on: http://gerrit.cloudera.org:8080/9015 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon--- M src/kudu/consensus/log_index.cc M src/kudu/util/env_posix.cc M src/kudu/util/net/socket.cc M src/kudu/util/os-util.h M src/kudu/util/subprocess-test.cc M src/kudu/util/subprocess.cc 6 files changed, 77 insertions(+), 22 deletions(-) Approvals: Kudu Jenkins: Verified Todd Lipcon: Looks good to me, approved -- 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: merged Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 Gerrit-Change-Number: 9015 Gerrit-PatchSet: 13 Gerrit-Owner: Jeffrey F. Lukman Gerrit-Reviewer: Jeffrey F. Lukman Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9015 ) Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. Patch Set 12: Code-Review+2 -- 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: 12 Gerrit-Owner: Jeffrey F. LukmanGerrit-Reviewer: Jeffrey F. Lukman Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 07:30:34 + Gerrit-HasComments: No
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Jeffrey F. Lukman has posted comments on this change. ( http://gerrit.cloudera.org:8080/9015 ) Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. Patch Set 12: (4 comments) http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/os-util.h File src/kudu/util/os-util.h: http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/os-util.h@27 PS10, Line 27: > should probably #include here since EINTR is defined there Done http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc File src/kudu/util/subprocess-test.cc: http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc@338 PS10, Line 338: ASSERT_TRUE(err == 0 || err == ESRCH); > nit: I think this would read better as ASSERT_TRUE(err == 0 || err == ESRCH Done http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc@340 PS10, Line 340: LOG(INFO) << "Async kill signal failed with err=" << err << > can you add an ASSERT_TRUE(t_finished) here? we should only get ESRCH in th Done http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc@344 PS10, Line 344: // Add microseconds delay to make the unit test runs faster and more reliable > can you change this to rand() % 1? It still runs quickly on my machine with Done -- 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: 12 Gerrit-Owner: Jeffrey F. LukmanGerrit-Reviewer: Jeffrey F. Lukman Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 06:44:32 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Hello Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9015 to look at the new patch set (#12). Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. KUDU-2208 Add RETRY_ON_EINTR() to Subprocess This patch submits a unit test that create a Subprocess thread and while it is starting and waiting, another thread sends kill signals to the Subprocess thread. Since the pthread_kill() signal is sent asynchronously, sometimes the pthread_kill() raises error signal 3. In that condition, the unit test logs the incident as an INFO. Prior to this bug fix patch, the Subprocess throws an exception because it cannot handle the kill signal in Wait() state. To fix the bug, we add RETRY_ON_EINTR() inside Subprocess::DoWait() function. Furthermore, this patch also moves the RETRY_ON_EINTR() function that occurs in many places across the code to os-util.h and it adds alarm reset in the end of TestReadFromStdoutAndStderr in Subprocess-test. Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 --- M src/kudu/consensus/log_index.cc M src/kudu/util/env_posix.cc M src/kudu/util/net/socket.cc M src/kudu/util/os-util.h M src/kudu/util/subprocess-test.cc M src/kudu/util/subprocess.cc 6 files changed, 77 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/9015/12 -- 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: newpatchset Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 Gerrit-Change-Number: 9015 Gerrit-PatchSet: 12 Gerrit-Owner: Jeffrey F. LukmanGerrit-Reviewer: Jeffrey F. Lukman Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Jeffrey F. Lukman has abandoned this change. ( http://gerrit.cloudera.org:8080/9042 ) Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. Abandoned Wrong commit -- To view, visit http://gerrit.cloudera.org:8080/9042 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Iffabada54eb2359730e9e2973b263d71645fa572 Gerrit-Change-Number: 9042 Gerrit-PatchSet: 1 Gerrit-Owner: Jeffrey F. LukmanGerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Jeffrey F. Lukman has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9042 Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. KUDU-2208 Add RETRY_ON_EINTR() to Subprocess This patch submits a unit test that create a Subprocess thread and while it is starting and waiting, another thread sends kill signals to the Subprocess thread. Since the pthread_kill() signal is sent asynchronously, sometimes the pthread_kill() raises error signal 3. In that condition, the unit test logs the incident as an INFO. Prior to this bug fix patch, the Subprocess throws an exception because it cannot handle the kill signal in Wait() state. To fix the bug, we add RETRY_ON_EINTR() inside Subprocess::DoWait() function. Furthermore, this patch also moves the RETRY_ON_EINTR() function that occurs in many places across the code to os-util.h and it adds alarm reset in the end of TestReadFromStdoutAndStderr in Subprocess-test Change-Id: Iffabada54eb2359730e9e2973b263d71645fa572 --- M src/kudu/util/os-util.h M src/kudu/util/subprocess-test.cc 2 files changed, 5 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/9042/1 -- To view, visit http://gerrit.cloudera.org:8080/9042 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Iffabada54eb2359730e9e2973b263d71645fa572 Gerrit-Change-Number: 9042 Gerrit-PatchSet: 1 Gerrit-Owner: Jeffrey F. Lukman
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9015 ) Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/os-util.h File src/kudu/util/os-util.h: http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/os-util.h@27 PS10, Line 27: #include should probably #include here since EINTR is defined there http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc File src/kudu/util/subprocess-test.cc: http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc@338 PS10, Line 338: ASSERT_FALSE(err != 0 && err != ESRCH); nit: I think this would read better as ASSERT_TRUE(err == 0 || err == ESRCH) http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc@340 PS10, Line 340: LOG(INFO) << "Async kill signal failed with err=" << err << can you add an ASSERT_TRUE(t_finished) here? we should only get ESRCH in the case where the thread finished http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc@344 PS10, Line 344: SleepFor(MonoDelta::FromMicroseconds(rand() % 100)); can you change this to rand() % 1? It still runs quickly on my machine with that setting and seems more likely to produce races -- 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: 10 Gerrit-Owner: Jeffrey F. LukmanGerrit-Reviewer: Jeffrey F. Lukman Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 06:16:51 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Hello Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9015 to look at the new patch set (#10). Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. KUDU-2208 Add RETRY_ON_EINTR() to Subprocess This patch submits a unit test that create a Subprocess thread and while it is starting and waiting, another thread sends kill signals to the Subprocess thread. Since the pthread_kill() signal is sent asynchronously, sometimes the pthread_kill() raises error signal 3. In that condition, the unit test logs the incident as an INFO. Prior to this bug fix patch, the Subprocess throws an exception because it cannot handle the kill signal in Wait() state. To fix the bug, we add RETRY_ON_EINTR() inside Subprocess::DoWait() function. Furthermore, this patch also moves the RETRY_ON_EINTR() function that occurs in many places across the code to os-util.h and it adds alarm reset in the end of TestReadFromStdoutAndStderr in Subprocess-test. Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 --- M src/kudu/consensus/log_index.cc M src/kudu/util/env_posix.cc M src/kudu/util/net/socket.cc M src/kudu/util/os-util.h M src/kudu/util/subprocess-test.cc M src/kudu/util/subprocess.cc 6 files changed, 74 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/9015/10 -- 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: newpatchset Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 Gerrit-Change-Number: 9015 Gerrit-PatchSet: 10 Gerrit-Owner: Jeffrey F. LukmanGerrit-Reviewer: Jeffrey F. Lukman Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Hello Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9015 to look at the new patch set (#9). Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. KUDU-2208 Add RETRY_ON_EINTR() to Subprocess This patch submits a unit test that create a Subprocess thread and while it is starting and waiting, another thread sends kill signals to the Subprocess thread. Since the pthread_kill() signal is sent asynchronously, sometimes the pthread_kill() raises error signal 3. In that condition, the unit test logs the incident as an INFO. Prior to this bug fix patch, the Subprocess throws an exception because it cannot handle the kill signal in Wait() state. To fix the bug, we add RETRY_ON_EINTR() inside Subprocess::DoWait() function. Furthermore, this patch also moves the RETRY_ON_EINTR() function that occurs in many places across the code to os-util.h. Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 --- M src/kudu/consensus/log_index.cc M src/kudu/util/env_posix.cc M src/kudu/util/net/socket.cc M src/kudu/util/os-util.h M src/kudu/util/subprocess-test.cc M src/kudu/util/subprocess.cc 6 files changed, 73 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/9015/9 -- 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: newpatchset Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 Gerrit-Change-Number: 9015 Gerrit-PatchSet: 9 Gerrit-Owner: Jeffrey F. LukmanGerrit-Reviewer: Jeffrey F. Lukman Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon