Skip to content
This repository has been archived by the owner on Apr 21, 2023. It is now read-only.

LoadFromFileCacheTtlMs is not honored (*after* first request) #1552

Merged
merged 4 commits into from
Jun 4, 2017

Conversation

oschaaf
Copy link
Member

@oschaaf oschaaf commented Jun 2, 2017

Not ready to go in yet, needs more verification of code paths.

Fixes apache/incubator-pagespeed-ngx#1418
Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch. It shouldn't be too hard to add a unit test, I think.


// Determine if we need to use the implicit cache ttl ms or the implicit
// load from file cache ttl ms.
int64 implicit_ttl = Options()->implicit_cache_ttl_ms();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implicit_ttl_ms (paranoid about units I am :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@oschaaf
Copy link
Member Author

oschaaf commented Jun 3, 2017

@jmarantz I changed an existing unit test, WDYT?

@@ -454,6 +461,16 @@ void InPlaceRewriteContext::RemoveRedundantRelCanonicalHeader(
ResponseHeaders::RelCanonicalHeaderValue(url_));
}

bool InPlaceRewriteContext::IsLoadFromFileBased() {
Copy link
Member Author

@oschaaf oschaaf Jun 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmarantz added this, I anticipate we may need it later for landing #1492

@0x42h
Copy link

0x42h commented Jun 3, 2017

Hi @oschaaf. Does this mean it's in master already? I'm new here.

Strike that. I just read...

Only those with write access to this repository can merge pull requests.

@oschaaf
Copy link
Member Author

oschaaf commented Jun 3, 2017

@0x42h It's not in master yet, this code change is currently under review. If everyone is happy with it it will get merged to master.

@oschaaf oschaaf merged commit c122121 into master Jun 4, 2017
@oschaaf oschaaf deleted the oschaaf-nps-issue-1418 branch June 4, 2017 07:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants