fetcher: httpcaching fixes #2965
Conversation
a3fcbf9 to
b84328e
Compare
| return "" | ||
| } | ||
|
|
||
| func getRemoteForURL(s *store.Store, u *url.URL) (*store.Remote, error) { |
There was a problem hiding this comment.
please, no more getters ;-)
There was a problem hiding this comment.
Just moved the original function 😄 ok for remoteForURL?
|
small nit, else LGTM |
|
... modulo green ;-) |
b84328e to
0896dc0
Compare
|
FWIW my plugin proposal also fixed the duplication, as after performing discovery it used the http(s) plugin to actually perform the fetching. |
The fetcher refactor in rkt#1763 moved Cache-Control max-age check inside the store caching logic, instead it should live in the transport logic (currently colled "remote"), like the ETag handling. This made the behavior different from the explained image fetching behavior and will cause redownloading an image when using `--no-store` regardless of the Cache-Control max-age value. Also fix the maybeUseCached function since it's wrong to check the returned cache-control max-age information. The check should be done with the current max-age saved in the remote (before the http fetching). A functional test has been added.
The fetcher refactor in rkt#1763 removed the http caching checks but left them only in the http fetcher. This patch adds the same logic also to the namefetcher and extends the functional test to also check fetching by appc name. In the meantime (it won't affect anything since there's no way to currently ask for it) change the meaning of Fetcher.NoCache to not disable both store and transport cache but only disable transport cache. This is a more logical one and decoupled from the store as it should be.
0896dc0 to
292a009
Compare
|
@lucab rebased. |
|
Is |
No, it's fine here. I was just wondering. |
|
lgtm |
|
I'll wait for @dgonyeo's review before merging though. |
|
LGTM |
The fetchers refactor in #1763 changed some logic in handling http Cache-Control max-age. The two commits fixes some issues for httpfetcher and namefetcher and adds related functional tests.
As you can see there's a lot of duplicated code between namefetcher and httpfetcher since namefetcher once discovered the ACI URL needs to fetch them via http (at the moment the unique supported transport). The proposals in #2953 and #2964 will avoid this code duplication.
The namefetcher commit will probably fix the problem reported in #2276 (comment)
/cc @dgonyeo It might interest you