Docserver: Add more skip_not_found support and cache "not found"s

Docserver: Add more skip_not_found support and cache "not found"s

BUG=305280
NOTRY=True

Committed: https://crrev.com/e8fc5eb6f26108bd00f85108e1ec4272e6196481
Cr-Commit-Position: refs/heads/master@{#292212}

[ahernandez, 2014-08-26 20:08:37]

ahernandez.miralles@gmail.com changed reviewers:
+ kalman@chromium.org

[ahernandez, 2014-08-26 20:08:37]

I'm submitting the skip_not_found changes in a separate CL. PTAL.

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/caching_file_system_test.py (right):

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system_test.py:237: def
testSkipNotFound(self):
This test wasn't even running before because of improper indentation!

[not at google - send to devlin, 2014-08-26 20:59:35]

Nice patch. Caching file system stuff is some of the most complicated/subtle
code in the whole docserver. A few nits + requests for tests.

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/caching_file_system.py (right):

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system.py:79: '''Reads a list
of files. If a file is in memcache and it is not out of
Could you change "memcache" to just "cached"?

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system.py:108: fresh_data =
dict(
This "fresh" thing is tripping me up. Maybe it should be "uptodate_data".

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system.py:110: if version and
stat_futures[path].Get().version == version)
I'm paranoid about version being 0 here. Could you do the "version is not None"
version of this check here?

Also mention in the comment *why* we're caching None as a value.

Which makes me wonder - do we need to do this? If we're already caching this
data in Stat, can you use that instead? It seems like that's exactly what you're
doing below.

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system.py:113: # Remove paths
for files that don't exist so reads aren't attempted.
Could you mention why this works? Namely that this method turns any Stat
FileNotFoundErrors into None, so we can check its existence that way.

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system.py:115: if
cached_read_values.get(path, (None, True))[1]]
Could you use 'path in cached_read_values'? This .get() form looks kinda
roundabout.

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system.py:127: # constantly
trying to read a file we now know doesn't exist.
Relating to comments above - I'm not sure we need to cache this, since the stat
cache already knows something similar. You could just update the results
directly, right?

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/caching_file_system_test.py (right):

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system_test.py:175:
self.assertRaises(FileNotFoundError, future.Get)
Nice tests. Is it possible to extend the test to check that incrementing the
stat for the directory will re-read the file? And that if the file happens to
appear, it picks it up?

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system_test.py:237: def
testSkipNotFound(self):
On 2014/08/26 20:08:37, ahernandez wrote:
> This test wasn't even running before because of improper indentation!

Oh man :\ good catch.

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/compiled_file_system_test.py (right):

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/compiled_file_system_test.py:220:
self.assertRaises(FileNotFoundError, future.Get)
Same comment in the other test, if possible.

[ahernandez, 2014-08-27 00:07:23]

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/caching_file_system.py (right):

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system.py:115: if
cached_read_values.get(path, (None, True))[1]]
On 2014/08/26 20:59:34, kalman wrote:
> Could you use 'path in cached_read_values'? This .get() form looks kinda
> roundabout.

The reason I have the ugly .get() call is because I'm only filtering out paths
that are in cached_read_values *and* have a version == None. I can't use 'path
in cached_read_values' or a variation on that because that would filter out
paths that are valid but just haven't been cached.

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system.py:127: # constantly
trying to read a file we now know doesn't exist.
On 2014/08/26 20:59:34, kalman wrote:
> Relating to comments above - I'm not sure we need to cache this, since the
stat
> cache already knows something similar. You could just update the results
> directly, right?

I think the stat cache doesn't know about non-existent files, because the
stat_futures for those files are normally not called. What I can do is add a
loop that runs through all the futures and calls them, so that the stat_cache is
updated, and then I'm pretty sure I can get away with not having to
unnecessarily add to the read cache, if that's preferable. Then I shouldn't have
to check for the version being None in the fresh_data dict comp.

[not at google - send to devlin, 2014-08-27 01:23:56]

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/caching_file_system.py (right):

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system.py:115: if
cached_read_values.get(path, (None, True))[1]]
On 2014/08/27 00:07:22, ahernandez wrote:
> On 2014/08/26 20:59:34, kalman wrote:
> > Could you use 'path in cached_read_values'? This .get() form looks kinda
> > roundabout.
> 
> The reason I have the ugly .get() call is because I'm only filtering out paths
> that are in cached_read_values *and* have a version == None. I can't use 'path
> in cached_read_values' or a variation on that because that would filter out
> paths that are valid but just haven't been cached.

Ah I see. So to be clear: If the item isn't cached yet then it'll return (None,
True), then we grab the True part and the path passes? Ok.

Could you add what you said as a comment?

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system.py:127: # constantly
trying to read a file we now know doesn't exist.
On 2014/08/27 00:07:22, ahernandez wrote:
> On 2014/08/26 20:59:34, kalman wrote:
> > Relating to comments above - I'm not sure we need to cache this, since the
> stat
> > cache already knows something similar. You could just update the results
> > directly, right?
> 
> I think the stat cache doesn't know about non-existent files, because the
> stat_futures for those files are normally not called. What I can do is add a
> loop that runs through all the futures and calls them, so that the stat_cache
is
> updated, and then I'm pretty sure I can get away with not having to
> unnecessarily add to the read cache, if that's preferable. Then I shouldn't
have
> to check for the version being None in the fresh_data dict comp.

Ohh I see. Ok. What you have is fine.

We should also explain somewhere that the read cache holds files that we know
don't exist by using a None value for their cached stat value. Maybe when it's
declared?

And now I understand the "if version ..." check up on line 110. You should
comment (so many comments, ah well, this file is complicated) that the "version
is not None" check is again because we cache not-found files as a None version.

[ahernandez, 2014-08-27 17:33:28]

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/caching_file_system.py (right):

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system.py:127: # constantly
trying to read a file we now know doesn't exist.
On 2014/08/27 01:23:55, kalman wrote:
> On 2014/08/27 00:07:22, ahernandez wrote:
> > On 2014/08/26 20:59:34, kalman wrote:
> > > Relating to comments above - I'm not sure we need to cache this, since the
> > stat
> > > cache already knows something similar. You could just update the results
> > > directly, right?
> > 
> > I think the stat cache doesn't know about non-existent files, because the
> > stat_futures for those files are normally not called. What I can do is add a
> > loop that runs through all the futures and calls them, so that the
stat_cache
> is
> > updated, and then I'm pretty sure I can get away with not having to
> > unnecessarily add to the read cache, if that's preferable. Then I shouldn't
> have
> > to check for the version being None in the fresh_data dict comp.
> 
> Ohh I see. Ok. What you have is fine.
> 
> We should also explain somewhere that the read cache holds files that we know
> don't exist by using a None value for their cached stat value. Maybe when it's
> declared?
> 
> And now I understand the "if version ..." check up on line 110. You should
> comment (so many comments, ah well, this file is complicated) that the
"version
> is not None" check is again because we cache not-found files as a None
version.

I don't think we should be afraid of comments :)

[ahernandez, 2014-08-27 18:54:38]

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/caching_file_system_test.py (right):

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system_test.py:175:
self.assertRaises(FileNotFoundError, future.Get)
On 2014/08/26 20:59:34, kalman wrote:
> Nice tests. Is it possible to extend the test to check that incrementing the
> stat for the directory will re-read the file? And that if the file happens to
> appear, it picks it up?

I'm not sure I can do that, because Stat updates are not caught once they've
been cached -- there's even a test to verify that behavior.

[not at google - send to devlin, 2014-08-27 19:22:13]

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/caching_file_system_test.py (right):

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system_test.py:175:
self.assertRaises(FileNotFoundError, future.Get)
On 2014/08/27 18:54:38, ahernandez wrote:
> On 2014/08/26 20:59:34, kalman wrote:
> > Nice tests. Is it possible to extend the test to check that incrementing the
> > stat for the directory will re-read the file? And that if the file happens
to
> > appear, it picks it up?
> 
> I'm not sure I can do that, because Stat updates are not caught once they've
> been cached -- there's even a test to verify that behavior.

Isn't this line 107 kinda?

[ahernandez, 2014-08-27 19:29:00]

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/caching_file_system_test.py (right):

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system_test.py:175:
self.assertRaises(FileNotFoundError, future.Get)
On 2014/08/27 19:22:13, kalman wrote:
> On 2014/08/27 18:54:38, ahernandez wrote:
> > On 2014/08/26 20:59:34, kalman wrote:
> > > Nice tests. Is it possible to extend the test to check that incrementing
the
> > > stat for the directory will re-read the file? And that if the file happens
> to
> > > appear, it picks it up?
> > 
> > I'm not sure I can do that, because Stat updates are not caught once they've
> > been cached -- there's even a test to verify that behavior.
> 
> Isn't this line 107 kinda?

That does increment the stat, but the stat_cache doesn't get updated, because it
already has a cached value for the directory. In fact, I think those tests are
kinda pointless because creating a new empty caching file system clears the
caches, so the resource would be re-fetched regardless of the stat, right?

[not at google - send to devlin, 2014-08-27 19:32:49]

Ok, never mind about the extra tests.

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/caching_file_system_test.py (right):

https://codereview.chromium.org/512453002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system_test.py:175:
self.assertRaises(FileNotFoundError, future.Get)
On 2014/08/27 19:29:00, ahernandez wrote:
> On 2014/08/27 19:22:13, kalman wrote:
> > On 2014/08/27 18:54:38, ahernandez wrote:
> > > On 2014/08/26 20:59:34, kalman wrote:
> > > > Nice tests. Is it possible to extend the test to check that incrementing
> the
> > > > stat for the directory will re-read the file? And that if the file
happens
> > to
> > > > appear, it picks it up?
> > > 
> > > I'm not sure I can do that, because Stat updates are not caught once
they've
> > > been cached -- there's even a test to verify that behavior.
> > 
> > Isn't this line 107 kinda?
> 
> That does increment the stat, but the stat_cache doesn't get updated, because
it
> already has a cached value for the directory. In fact, I think those tests are
> kinda pointless because creating a new empty caching file system clears the
> caches, so the resource would be re-fetched regardless of the stat, right?

Ah yes I think you're right, those tests are ... pointless. Ok. Perhaps the
intention was to re-use the object store. Whatevs.

[ahernandez, 2014-08-27 20:15:55]

New patch set is up. Did you want those superfluous tests removed?

[not at google - send to devlin, 2014-08-27 20:25:05]

On 2014/08/27 20:15:55, ahernandez wrote:
> New patch set is up. Did you want those superfluous tests removed?

Nah don't worry about it.

[not at google - send to devlin, 2014-08-27 20:26:25]

lgtm, but bump the yaml versions.

[ahernandez, 2014-08-27 20:27:15]

On 2014/08/27 20:26:25, kalman wrote:
> lgtm, but bump the yaml versions.

End version, right?

[not at google - send to devlin, 2014-08-27 20:28:10]

On 2014/08/27 20:27:15, ahernandez wrote:
> On 2014/08/27 20:26:25, kalman wrote:
> > lgtm, but bump the yaml versions.
> 
> End version, right?

I actually never ended up implementing the logic which made it any different :)

But if I *had* then this affects the content of the caches, so it should be the
middle version.

[ahernandez, 2014-08-27 20:30:49]

The CQ bit was checked by ahernandez.miralles@gmail.com

[commit-bot: I haz the power, 2014-08-27 20:32:12]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/ahernandez.miralles@gmail.com/51245300...

[commit-bot: I haz the power, 2014-08-27 20:33:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-27 20:33:47]

Failed to commit the patch.

[ahernandez, 2014-08-27 20:37:18]

The CQ bit was checked by ahernandez.miralles@gmail.com

[commit-bot: I haz the power, 2014-08-27 20:39:09]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/ahernandez.miralles@gmail.com/51245300...

[commit-bot: I haz the power, 2014-08-27 20:40:15]

Committed patchset #3 (id:40001) as 93a7d53a576549f60dc691d9fea9a346003e17b4

[commit-bot: I haz the power, 2014-09-10 02:53:41]

Patchset 3 (id:??) landed as
https://crrev.com/e8fc5eb6f26108bd00f85108e1ec4272e6196481
Cr-Commit-Position: refs/heads/master@{#292212}