Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout

[Eric Sunshine]

On Tue, Nov 17, 2015 at 4:34 AM, Luke Diamand  wrote:
>>> 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

[Luke Diamand]

>>
>> 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

[Lars Schneider]

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.

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

[Luke Diamand]

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

[Lars Schneider]

On 16 Nov 2015, at 09:36, Luke Diamand  wrote:

> 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

[Luke Diamand]

On 17/11/15 08:22, Lars Schneider wrote:


On 16 Nov 2015, at 09:36, Luke Diamand  wrote:


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

[Eric Sunshine]

On Tue, Nov 17, 2015 at 3:28 AM, Lars Schneider
 wrote:
> 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

[Eric Sunshine]

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

[Luke Diamand]

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?


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