remove --http-cache-dir option ?

RESOLVED FIXED

Status

Testing
mozregression
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: parkouss, Assigned: parkouss)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox43 affected)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
I personally never use the --http-cache-dir option, and I think it is not useful.

I don't see a great performance win with that option enabled, I doubt somebody uses that ? This is now completely useless for inbounds, as we use task cluster, and nighlies will be using taskcluster at some point too (maybe not for old builds though).

So I propose to remove that option, the related code and the dependency needed for it.
(Assignee)

Comment 1

3 years ago
Will, Elbart, what do you think of it ? Do you use that option, and/or find it useful ?
Flags: needinfo?(wlachance)
Flags: needinfo?(elbart)

Comment 2

3 years ago
Wasn't it added in case somebody retried a build without a persist-folder, in which case mozreg wouldn't unnecessarily redownload the build again?

I've never used it as an option to define a folder, as I figured that's what persist already covers.
Flags: needinfo?(elbart)
(Assignee)

Comment 3

3 years ago
(In reply to Elbart from comment #2)
> Wasn't it added in case somebody retried a build without a persist-folder,
> in which case mozreg wouldn't unnecessarily redownload the build again?

Well it helped - because we had no proper solution at the time. Now this is handled in another way.

> I've never used it as an option to define a folder, as I figured that's what
> persist already covers.

Cool, nice to know that. I also think that persist folder is what we want. :)
(In reply to Julien Pagès from comment #3)
> (In reply to Elbart from comment #2)
> > Wasn't it added in case somebody retried a build without a persist-folder,
> > in which case mozreg wouldn't unnecessarily redownload the build again?
> 
> Well it helped - because we had no proper solution at the time. Now this is
> handled in another way.

How do we prevent additional inbound downloads now, if the user re-downloads a build (e.g. from running mozregression a second time)? I don't see how taskcluster helps with that.
Flags: needinfo?(wlachance)
(Assignee)

Comment 5

3 years ago
Will, I think what you are talking about is --persist, that allows to keep builds in a folder.

But builds that the user try again inside a run are no more re downloaded:

https://github.com/mozilla/mozregression/blob/master/mozregression/bisector.py#L490

The http cache utility is only useful for caching http requests to url we are visiting (ie list urls inside a web page), but I think it is not really useful.

taskcluster helps because we are no more crawling web pages. Task cluster give us the url, and we download the file, that's all.
(In reply to Julien Pagès from comment #5)
> Will, I think what you are talking about is --persist, that allows to keep
> builds in a folder.

Nope. The whole intent of the --http-cache was to automatically and transparently keep a cache of downloaded builds *between* runs of mozregression: see bug 1019528. I think this functionality is useful and we should keep it-- perhaps even make it the default. Redownloading builds is pretty time consuming.
(Assignee)

Comment 7

3 years ago
So we discussed more this subject with Will. We came up with:

 - remove the http-cache-dir options
 - make a persist folder by default (no need to use it on cmdline or to configure in the configuration file)
 - add an option to limit the size of the persist folder (with a good default, like probably 1G)

So let's can keep this bug to remove the http-cache-dir option.
(Assignee)

Updated

3 years ago
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
(Assignee)

Comment 8

3 years ago
Created attachment 8649741 [details] [review]
Remove http cache

MikeLing, I ask you for this review. This is mainly code removal, just added one test because the code was covered indirectly by some test I removed related to linitedfilecache.
Attachment #8649741 - Flags: review?(sabergeass)

Comment 9

3 years ago
Comment on attachment 8649741 [details] [review]
Remove http cache

I just test the PR locally and I think it works fine, thank you for your trust! :)
Attachment #8649741 - Flags: review?(sabergeass) → review+
You need to log in before you can comment on or make changes to this bug.