Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout
On Tue, Nov 17, 2015 at 4:34 AM, Luke Diamandwrote: >>> While implementing it I thought more about it. P4D is only >>> supported on platforms that support the date function. That means >>> these tests will only run on platforms that support the date >>> function. Consequently I wondered if this would justify the >>> slightly more complicated code. However, if you think this change >>> would help the patch to get accepted then I will add it. >> >> I don't feel strongly about it, and it's not my call anyhow. Opinions >> of Junio, Peff (as interim maintainer), and Luke weigh much more >> heavily than my own. Punting on dynamic detection of "date +%s" may be >> perfectly acceptable with the attitude that it can be implemented >> later if someone runs across a case where it's actually needed. > > Which other platforms are we talking about here? > > https://www.perforce.com/downloads/helix > > From there, you can get Solaris10, HP-UX, AIX and various flavours of BSD. > Solaris supports "date +%s". The question about "date +%s" portability arose with a suggestion to employ it in git-filter-branch[1]. Apparently, the concern about Solaris was raised in response to a Stack Overflow question[2] which claimed that +%s was not supported by that OS. Unfortunately, however, [2] did not indicate to which (older?) versions of Solaris that shortcoming applied. If Solaris 10 does support +%s, and the Perforce product is only available for Solaris 10, then perhaps concern about +%s a non-issue(?). [1]: http://git.661346.n2.nabble.com/FEATURE-REQUEST-Filter-branch-extend-progress-with-a-simple-estimated-time-remaning-td7638200.html#a7638504 [2]: http://stackoverflow.com/questions/2445198/get-seconds-since-epoch-in-any-posix-compliant-shell -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout
>> >> Which other platforms are we talking about here? >> >> https://www.perforce.com/downloads/helix >> >> From there, you can get Solaris10, HP-UX, AIX and various flavours of BSD. >> Solaris supports "date +%s". > > The question about "date +%s" portability arose with a suggestion to > employ it in git-filter-branch[1]. Apparently, the concern about > Solaris was raised in response to a Stack Overflow question[2] which > claimed that +%s was not supported by that OS. Unfortunately, however, > [2] did not indicate to which (older?) versions of Solaris that > shortcoming applied. If Solaris 10 does support +%s, and the Perforce > product is only available for Solaris 10, then perhaps concern about > +%s a non-issue(?). > > [1]: > http://git.661346.n2.nabble.com/FEATURE-REQUEST-Filter-branch-extend-progress-with-a-simple-estimated-time-remaning-td7638200.html#a7638504 > [2]: > http://stackoverflow.com/questions/2445198/get-seconds-since-epoch-in-any-posix-compliant-shell Rereading the man page more carefully, you're right, Solaris 10 doesn't do "%s". Using python to get the seconds should work though. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout
On 16 Nov 2015, at 22:14, Eric Sunshinewrote: > On Sun, Nov 15, 2015 at 8:08 AM, wrote: >> From: Lars Schneider >> >> In rare cases kill/cleanup operations in tests fail. Retry these >> operations with a timeout to make the test less flaky. >> >> Signed-off-by: Lars Schneider >> --- >> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh >> @@ -121,22 +125,33 @@ p4_add_user() { >>EOF >> } >> >> +retry_until_success() { >> +timeout=$(($(date +%s) + $RETRY_TIMEOUT)) > > There was some discussion previously[1] about detecting dynamically > whether 'date +%s' was supported. Was this something that you intended > to do, or did you decide against it since p4 is not supported on such > platforms? > > Same question also applies to patch 4/6. While implementing it I thought more about it. P4D is only supported on platforms that support the date function. That means these tests will only run on platforms that support the date function. Consequently I wondered if this would justify the slightly more complicated code. However, if you think this change would help the patch to get accepted then I will add it. Thanks, Lars > > [1]: > http://article.gmane.org/gmane.comp.version-control.git/280978/match=lazy+prerequisite > >> +until "$@" 2>/dev/null || test $(date +%s) -gt $timeout >> +do : >> +done >> +} >> + >> +retry_until_fail() { >> +timeout=$(($(date +%s) + $RETRY_TIMEOUT)) >> +until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout >> +do : >> +done >> +} -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout
While implementing it I thought more about it. P4D is only supported on platforms that support the date function. That means these tests will only run on platforms that support the date function. Consequently I wondered if this would justify the slightly more complicated code. However, if you think this change would help the patch to get accepted then I will add it. I don't feel strongly about it, and it's not my call anyhow. Opinions of Junio, Peff (as interim maintainer), and Luke weigh much more heavily than my own. Punting on dynamic detection of "date +%s" may be perfectly acceptable with the attitude that it can be implemented later if someone runs across a case where it's actually needed. Which other platforms are we talking about here? https://www.perforce.com/downloads/helix From there, you can get Solaris10, HP-UX, AIX and various flavours of BSD. Solaris supports "date +%s". HP-UX and AIX, I really don't know. Windows? I assume 'date +%s' will work for people using mingw. Is it possible to get the time in seconds by doing something like this: time_in_seconds() { python -c 'import time; print time.time()' } Luke -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout
On 16 Nov 2015, at 09:36, Luke Diamandwrote: > On 15/11/15 13:08, larsxschnei...@gmail.com wrote: >> From: Lars Schneider >> >> In rare cases kill/cleanup operations in tests fail. Retry these >> operations with a timeout to make the test less flaky. > > Should there be a sleep in that retry_until_success loop so that it doesn't > spin sending signals to p4d? Agreed. I will add a sleep in the next roll! > > Do we need to worry about the time offset being updated (e.g. NTP) while this > is running? Interesting question! I would consider this an edge case but I can see how it could happen. Do you see a way to handle that in an easy way? Thanks, Lars > >> >> Signed-off-by: Lars Schneider >> --- >> t/lib-git-p4.sh | 31 +++ >> 1 file changed, 23 insertions(+), 8 deletions(-) >> >> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh >> index 7548225..8d6b48f 100644 >> --- a/t/lib-git-p4.sh >> +++ b/t/lib-git-p4.sh >> @@ -6,6 +6,10 @@ >> # a subdirectory called "$git" >> TEST_NO_CREATE_REPO=NoThanks >> >> +# Some operations require multiple attempts to be successful. Define >> +# here the maximal retry timeout in seconds. >> +RETRY_TIMEOUT=60 >> + >> . ./test-lib.sh >> >> if ! test_have_prereq PYTHON >> @@ -121,22 +125,33 @@ p4_add_user() { >> EOF >> } >> >> +retry_until_success() { >> +timeout=$(($(date +%s) + $RETRY_TIMEOUT)) >> +until "$@" 2>/dev/null || test $(date +%s) -gt $timeout >> +do : > > sleep here? > >> +done >> +} >> + >> +retry_until_fail() { >> +timeout=$(($(date +%s) + $RETRY_TIMEOUT)) >> +until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout >> +do : > > sleep here? > >> +done >> +} >> + >> kill_p4d() { >> pid=$(cat "$pidfile") >> -# it had better exist for the first kill >> -kill $pid && >> -for i in 1 2 3 4 5 ; do >> -kill $pid >/dev/null 2>&1 || break >> -sleep 1 >> -done && >> +retry_until_fail kill $pid >> +retry_until_fail kill -9 $pid >> # complain if it would not die >> test_must_fail kill $pid >/dev/null 2>&1 && >> rm -rf "$db" "$cli" "$pidfile" >> } >> >> cleanup_git() { >> -rm -rf "$git" && >> -mkdir "$git" >> +retry_until_success rm -r "$git" >> +test_must_fail test -d "$git" && >> +retry_until_success mkdir "$git" >> } >> >> marshal_dump() { -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout
On 17/11/15 08:22, Lars Schneider wrote: On 16 Nov 2015, at 09:36, Luke Diamandwrote: On 15/11/15 13:08, larsxschnei...@gmail.com wrote: From: Lars Schneider In rare cases kill/cleanup operations in tests fail. Retry these operations with a timeout to make the test less flaky. Should there be a sleep in that retry_until_success loop so that it doesn't spin sending signals to p4d? Agreed. I will add a sleep in the next roll! Do we need to worry about the time offset being updated (e.g. NTP) while this is running? Interesting question! I would consider this an edge case but I can see how it could happen. Do you see a way to handle that in an easy way? You want to somehow call clock_gettime(CLOCK_MONOTONIC). That's not in python until 3.3. Writing a C program seems like overkill but could be a solution if this becomes a problem. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout
On Tue, Nov 17, 2015 at 3:28 AM, Lars Schneiderwrote: > On 16 Nov 2015, at 22:14, Eric Sunshine wrote: >> On Sun, Nov 15, 2015 at 8:08 AM, wrote: >>> From: Lars Schneider >>> >>> In rare cases kill/cleanup operations in tests fail. Retry these >>> operations with a timeout to make the test less flaky. >>> >>> Signed-off-by: Lars Schneider >>> --- >>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh >>> @@ -121,22 +125,33 @@ p4_add_user() { >>>EOF >>> } >>> >>> +retry_until_success() { >>> +timeout=$(($(date +%s) + $RETRY_TIMEOUT)) >> >> There was some discussion previously[1] about detecting dynamically >> whether 'date +%s' was supported. Was this something that you intended >> to do, or did you decide against it since p4 is not supported on such >> platforms? >> >> Same question also applies to patch 4/6. > > While implementing it I thought more about it. P4D is only > supported on platforms that support the date function. That means > these tests will only run on platforms that support the date > function. Consequently I wondered if this would justify the > slightly more complicated code. However, if you think this change > would help the patch to get accepted then I will add it. I don't feel strongly about it, and it's not my call anyhow. Opinions of Junio, Peff (as interim maintainer), and Luke weigh much more heavily than my own. Punting on dynamic detection of "date +%s" may be perfectly acceptable with the attitude that it can be implemented later if someone runs across a case where it's actually needed. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout
On Sun, Nov 15, 2015 at 8:08 AM,wrote: > From: Lars Schneider > > In rare cases kill/cleanup operations in tests fail. Retry these > operations with a timeout to make the test less flaky. > > Signed-off-by: Lars Schneider > --- > diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh > @@ -121,22 +125,33 @@ p4_add_user() { > EOF > } > > +retry_until_success() { > +timeout=$(($(date +%s) + $RETRY_TIMEOUT)) There was some discussion previously[1] about detecting dynamically whether 'date +%s' was supported. Was this something that you intended to do, or did you decide against it since p4 is not supported on such platforms? Same question also applies to patch 4/6. [1]: http://article.gmane.org/gmane.comp.version-control.git/280978/match=lazy+prerequisite > +until "$@" 2>/dev/null || test $(date +%s) -gt $timeout > +do : > +done > +} > + > +retry_until_fail() { > +timeout=$(($(date +%s) + $RETRY_TIMEOUT)) > +until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout > +do : > +done > +} -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout
On 15/11/15 13:08, larsxschnei...@gmail.com wrote: From: Lars SchneiderIn rare cases kill/cleanup operations in tests fail. Retry these operations with a timeout to make the test less flaky. Should there be a sleep in that retry_until_success loop so that it doesn't spin sending signals to p4d? Do we need to worry about the time offset being updated (e.g. NTP) while this is running? Signed-off-by: Lars Schneider --- t/lib-git-p4.sh | 31 +++ 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh index 7548225..8d6b48f 100644 --- a/t/lib-git-p4.sh +++ b/t/lib-git-p4.sh @@ -6,6 +6,10 @@ # a subdirectory called "$git" TEST_NO_CREATE_REPO=NoThanks +# Some operations require multiple attempts to be successful. Define +# here the maximal retry timeout in seconds. +RETRY_TIMEOUT=60 + . ./test-lib.sh if ! test_have_prereq PYTHON @@ -121,22 +125,33 @@ p4_add_user() { EOF } +retry_until_success() { +timeout=$(($(date +%s) + $RETRY_TIMEOUT)) +until "$@" 2>/dev/null || test $(date +%s) -gt $timeout +do : sleep here? +done +} + +retry_until_fail() { +timeout=$(($(date +%s) + $RETRY_TIMEOUT)) +until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout +do : sleep here? +done +} + kill_p4d() { pid=$(cat "$pidfile") - # it had better exist for the first kill - kill $pid && - for i in 1 2 3 4 5 ; do - kill $pid >/dev/null 2>&1 || break - sleep 1 - done && + retry_until_fail kill $pid + retry_until_fail kill -9 $pid # complain if it would not die test_must_fail kill $pid >/dev/null 2>&1 && rm -rf "$db" "$cli" "$pidfile" } cleanup_git() { - rm -rf "$git" && - mkdir "$git" + retry_until_success rm -r "$git" + test_must_fail test -d "$git" && + retry_until_success mkdir "$git" } marshal_dump() { -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout
From: Lars SchneiderIn rare cases kill/cleanup operations in tests fail. Retry these operations with a timeout to make the test less flaky. Signed-off-by: Lars Schneider --- t/lib-git-p4.sh | 31 +++ 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh index 7548225..8d6b48f 100644 --- a/t/lib-git-p4.sh +++ b/t/lib-git-p4.sh @@ -6,6 +6,10 @@ # a subdirectory called "$git" TEST_NO_CREATE_REPO=NoThanks +# Some operations require multiple attempts to be successful. Define +# here the maximal retry timeout in seconds. +RETRY_TIMEOUT=60 + . ./test-lib.sh if ! test_have_prereq PYTHON @@ -121,22 +125,33 @@ p4_add_user() { EOF } +retry_until_success() { +timeout=$(($(date +%s) + $RETRY_TIMEOUT)) +until "$@" 2>/dev/null || test $(date +%s) -gt $timeout +do : +done +} + +retry_until_fail() { +timeout=$(($(date +%s) + $RETRY_TIMEOUT)) +until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout +do : +done +} + kill_p4d() { pid=$(cat "$pidfile") - # it had better exist for the first kill - kill $pid && - for i in 1 2 3 4 5 ; do - kill $pid >/dev/null 2>&1 || break - sleep 1 - done && + retry_until_fail kill $pid + retry_until_fail kill -9 $pid # complain if it would not die test_must_fail kill $pid >/dev/null 2>&1 && rm -rf "$db" "$cli" "$pidfile" } cleanup_git() { - rm -rf "$git" && - mkdir "$git" + retry_until_success rm -r "$git" + test_must_fail test -d "$git" && + retry_until_success mkdir "$git" } marshal_dump() { -- 2.5.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html