Tails

#1 Updated by intrigeri 2016-07-18 07:23:47

• related to Bug #10494: Retry htpdate when it fails added

#2 Updated by intrigeri 2016-07-18 08:03:19

• Assignee changed from intrigeri to bertagaz
• % Done changed from 50 to 60
• QA Check changed from Ready for QA to Info Needed

It seems to me that the --max-time option might be more adequate than --connect-timeout, to reach the goal set in commit:ce71966252f97c1c17bcef3d52d049c7ca6e122c. I mean what we want is to lower the time a call to curl can take, and we don’t really care about what time the connect phase in particular takes, right?

Other than that: (static) code review passes!

#3 Updated by bertagaz 2016-07-19 05:47:15

• Assignee changed from bertagaz to intrigeri

intrigeri wrote:
> It seems to me that the --max-time option might be more adequate than --connect-timeout, to reach the goal set in commit:ce71966252f97c1c17bcef3d52d049c7ca6e122c. I mean what we want is to lower the time a call to curl can take, and we don’t really care about what time the connect phase in particular takes, right?

Well, I don’t think it changes much, as if CURL fails in this case it’s because it can’t connect to the HTTP server. With max-time, I guess in theory CURL could take a bit more than 29 seconds to connect and miss the answer because it took a bit too much time to receive it, but I don’t think in practice it is a real problem.

I’ve tested a bit this branch with --max-time and it doesn’t seem to break the behavior anyway, so I’ve pushed commit:c4b0438 (unmerged in other related branches though). Still this option is less tested than the connect-timeout one. It’s still possible to revert the commit and reintroduce it for 2.6, if we prefer to test it a bit more. Note that this commit makes sense with Bug #10494#note-47 as we’d have to use this option.

Now I think note Bug #10494#note-42 is still a blocker for a merge: with 30 seconds CURL timeout, we only try for half the duration of the ‘time has synced’ try_for(). So either we bump this timeout to 1 minute, or we lower the try_for(), or we make htpdate retry only 1 time.

#4 Updated by intrigeri 2016-07-19 08:32:04

• Assignee changed from intrigeri to bertagaz

> Now I think note Bug #10494#note-42 is still a blocker for a merge: with 30 seconds CURL timeout, we only try for half the duration of the ‘time has synced’ try_for(). So either we bump this timeout to 1 minute, or we lower the try_for(), or we make htpdate retry only 1 time.

I don’t understand this part. As far as I understood the problem, we need to make sure that the test suite waits at least as long as htpdate can possibly run, and it seems that you’re confirming this will be finally satisfied once we merge this branch, which is an improvement. What other condition do we need to satisfy, and why?

#5 Updated by bertagaz 2016-07-20 03:26:56

• Assignee changed from bertagaz to intrigeri
• QA Check changed from Info Needed to Ready for QA

intrigeri wrote:
> I don’t understand this part. As far as I understood the problem, we need to make sure that the test suite waits at least as long as htpdate can possibly run, and it seems that you’re confirming this will be finally satisfied once we merge this branch, which is an improvement. What other condition do we need to satisfy, and why?

Well, I thought this try_for() were somehow describing the maximum amount of time an action could take. Seems I misunderstood it. It just feels a bit odd to wait for 5 minutes for something we know won’t take more than a half of it. Anyway, your call.

#6 Updated by intrigeri 2016-07-20 10:19:45

• Assignee changed from intrigeri to bertagaz
• QA Check changed from Ready for QA to Dev Needed

Please reassign to me once the change is in upstream htp.git.

> It just feels a bit odd to wait for 5 minutes for something we know won’t take more than a half of it.

Potentially wasting a couple minutes test suite runtime here doesn’t feel like a merge blocker to me. I bet there are more worthwhile places to work into optimizing test suite runtime. But whatever, if you want to work on this, feel free to, as long as this doesn’t make it more fragile (in this case, the 2.5 minutes seems too simplistic, as the whole thing can take slightly more that, so it’ll need a bit more guesswork and thought, which is precisely what IMO the problem at hand does not deserve).

#7 Updated by bertagaz 2016-07-22 02:01:03

• Assignee changed from bertagaz to intrigeri
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> Please reassign to me once the change is in upstream htp.git.

Ooops, seems I can’t get to remind there’s this other repo. Done.

> > It just feels a bit odd to wait for 5 minutes for something we know won’t take more than a half of it.
>
> Potentially wasting a couple minutes test suite runtime here doesn’t feel like a merge blocker to me. I bet there are more worthwhile places to work into optimizing test suite runtime. But whatever, if you want to work on this, feel free to, as long as this doesn’t make it more fragile (in this case, the 2.5 minutes seems too simplistic, as the whole thing can take slightly more that, so it’ll need a bit more guesswork and thought, which is precisely what IMO the problem at hand does not deserve).

Fair enough, not a merge blocker then. Future work on Bug #10494 can probably take care of that anyway.

#8 Updated by intrigeri 2016-07-22 11:24:12

• Status changed from In Progress to Fix committed
• Assignee deleted (intrigeri)
• % Done changed from 60 to 100
• QA Check changed from Ready for QA to Pass
• Deliverable for changed from SponsorS_Internal to 270

Merged into stable, thanks!

#9 Updated by intrigeri 2016-08-02 09:29:49

• Status changed from Fix committed to Resolved