Extensions Docs Server: Internal file system

Extensions Docs Server: Internal file system

The docs server now uses an internal file system to handle fetching and caching
resources. This greatly speeds up fetching from SVN.

BUG=131095
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=147471

[cduvall, 2012-07-17 20:07:10]

Thanks for the review!

[not at google - send to devlin, 2012-07-18 10:39:15]

i didn't look at any of the tests, but this is enough comments for now.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/api_data_source_test.py:23: fetcher =
LocalFileSystem(self._base_path)
file_system =

or inline it.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/appengine_url_fetcher.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_url_fetcher.py:7: class
AppEngineUrlFetcher(object):
documentation

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_url_fetcher.py:11: def
fetch(self, url):
These should all be Google-style, like Fetch.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_url_fetcher.py:17: def
make_fetch_call(self, rpc, path):
This *is* the appengine API, I know; but it could be done a higher level
(particularly if we're going to make fake ones). It's just that this
create_rpc/make_fetch_call yuck is a pain to fake out.

Coud you have it be like, Fetch (calls .fetch directly) and FetchAsync (returns
a Future for a fetch value and does the create_rpc stuff itself).

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/async_fetch_value.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/async_fetch_value.py:7: class
AsyncFetchValue(object):
Hm, sorry, this is pretty tied into the FileSystem stuff. Specifically
SubversionFileSystem. Could you move it into there? And... err, rename it
_AsyncFetchFuture.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/async_fetch_value.py:38: if self._error is
not None:
what's wrong with !=

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/branch_utility.py:27: return channel_name
I posit that this isn't a number

(just sayin)

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/branch_utility.py:29: fetch_data =
urlfetch.fetch(base_path)
The result of this fetch should be cached. Just memcache it for an hour or so.

Though that'd mean you need to abstract away memcache for testing. Bleh. Maybe
you'll need to do that anyway to test memcache_file_system.

Btw please make this a class, injecting urlfetch into every call to
GetBranchNumberForChannelName is icky. Ditto base_path.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/branch_utility.py:31: raise
Exception('Fetch returned zero results.')
It'd crash anyway with an invalid JSON thing, so checking here is kinda moot. I
don't see why empty is any different to invalid json.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/branch_utility.py:46: sorted_list = [x for
x in branch_numbers.iteritems()]
x for x in blah... isn't that just blah?

If you're trying to listify, then list(branch_numbers.iteritems()) would do it.
then again, for that matter, so would branch_numbers.items().

then again, this whole expression can just be:
sorted_list = sorted(branch_numbers.iteritems(),
                     key=operator.itemgetter(1),
                     reversed=True)

also sorted_list is pretty vague, sorted_branches or something. whatever it's
supposed to be.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/branch_utility.py:47: sorted_list.sort(key
= lambda x: x[1])
key=itemgetter(1)

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/echo_handler.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/echo_handler.py:72: svn_fs =
SubversionFileSystem(fetcher)
inline svn_fs?

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/echo_handler.py:107:
self._GetInstanceForBranch(branch).Get(real_path, self)
musing: when branch versions bump, this leaks Server instances. We might want to
add some logic to detect that at some point. Add TODO?

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/example_zipper.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:13: def __init__(self,
fetcher, cache_builder, base_path, match_path):
s/fetcher/file_system/g

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:33: def create(self,
path):
s/create/Create/

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/fake_url_fetcher.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/fake_url_fetcher.py:7: class
FakeUrlFetcher(object):
See comment in appengine_url_fetcher; it would simplify this class a lot.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/fetcher_cache.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/fetcher_cache.py:8: class
FetcherCache(object):
probably needs a rename/redocument.

also, s/fetcher/file_system/g

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/fetcher_cache.py:23: class
_CacheEntry(object):
Here's the thing with this class now, by the way: the time-based caching is
redundant. And kinda of makes it... less good.

Basically, we should change the way this works, from having time-based cache
invalidation to querying Stat on the file system for whether it's expired.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/fetcher_cache.py:51: def getFromFile(self,
key):
document

and key -> path

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/fetcher_cache.py:62: def
getFromFileListing(self, key):
document

also key -> path

btw this method should be tolerant to input that doesn't end in a / ... either
that or crash if it does. I prefer the former.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/file_system.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/file_system.py:10: def Read(self, paths):
pass
raise NotImplementedError()

also newline after the method

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/file_system.py:11: def Stat(self, path):
documentation on all these. and on the classes.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/file_system.py:12: return 0
raise NotImplementedError()?

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/lazy_value.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/lazy_value.py:7: class LazyValue(object):
I realised today that this isn't *like* Java's Future. It *is* Java's Future.

So... call it Future. Sorry bout that :)

also: documentation

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/lazy_value.py:14: self._delegate is None):
what's wrong with ==

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/lazy_value.py:17: def Get(self):
documentation

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/lazy_value.py:20: if self._error is not
None:
what's wrong with !=

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/local_file_system.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/local_file_system.py:11: """Class to fetch
resources from local filesystem.
FileSystem implementation which fetches resources from the local filesystem.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/memcache_file_system.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:7: class
MemcacheFileSystem(object):
documentation etc. Why does this not extend FileSystem while LocalFileSystem
does? For documentation's sake this should extend FileSystem, I think.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:10: self._memcache
= memcache
Class needs tests! And for that you'll probably need to pull out memcache into
its own little abstraction thing (AppengineMemcache / InMemoryMemcache), though
it'd be much more trivial than the urlfetch thing. Very trivial.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:13: directory =
path.rsplit('/', 1)[0]
Stat should be at the path level, not the directory level. If somebody wants to
stat a directory they should convert the path into a directory first.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:14: stat_info =
self._memcache.get(directory, namespace='directories')
we should be more disciplined with these namespaces. either name them with a
stronger convention (e.g. here, 'MemcacheFileSystem.Stat', below,
'MemcacheFileSystem.Read'), or list them somewhere.

Actually what might be nice (and would have prevented those bugs below) is to
make your memcache abstraction have a compulsory namespace, and have those
listed. E.g.

class Memcache(object):
  MEMCACHE_FILE_SYSTEM_READ = "MemcacheFileSystem.Get"
  MEMCACHE_FILE_SYSTEM_STAT = "MemcacheFileSystem.Stat"

  def Get(self, key):
    raise NotImplementedError()

class AppengineMemcache(object):
  def Get(self, key, time=60, namespace=None):
    if namespace == None:
      raise ValueError(...)
    return memcache.get(key, time=time, namespace=namespace)

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:25: if
cached_result is None:
what's wrong with ==

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:29: if
self.Stat(path) > version:
So this means that if any file in a directory changes, all files in that
directory get invalidated. Hm. If Stat took a path, this logic *could* be

if (self.Stat(_GetDir(path)) > version and
    self.Stat(path) > version):
  ...

which wouldn't be a whole lot better since now there's twice the number of stats
(although they get cached, so, hm, maybe not).

My point is that this could be *even better* as:

data, version = cached_result
if self.Stat(_GetDir(path)) > version:
  # Some entities(s) in the directory has changed. It might be this one?
  entity_version = self.Stat(path).version
  if entity_version > version:
    # Entity has changed. Invalidate it and read the whole contents back later.
    memcache.Delete(path, namespace=...)
    uncached.append(path)
    continue
  # Entity *hasn't* changed, and that means we can bump its version
  # in memcache so that it doesn't need to be re-stat'ed next time.
  memcache.Set(path, (data, entity_version), namespace=...)
result[path] = Future(value=data)

p.s. try to document these nontrivial algorithms.

p.s. also my brain is getting a bit sore but this all seems a bit prone to race
conditions, if memcache can change underneath us. Something to look into.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:30:
self._memcache.delete(path)
also missing namespace

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:37:
self._memcache.set(item, (new_items[item], version), namespace='data')
set but never get?

Oh, the get is missing its namespace. Foundabug!

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:38:
new_items[item] = LazyValue(value=new_items[item])
why not
result[item] = LazyValue(value=new_items[item])
?
then you don't need the update, and don't need to be writing over the data
that's being iterated over (which looks strange)

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:25: [path +
'/manifest.json'])[path + '/manifest.json'].Get()
seems like you should hold onto path + '/manifest.json' in a variable.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:39: api_items =
set([])
don't need the [], just set()

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:40:
[self._GetApiItems(api_items, js.Get()) for js in js_contents.values()]
I preferred the way it was... side-effectful list comprehensions doesn't seem
like a good idiom.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/subversion_file_system.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_file_system.py:11: class
SubversionFileSystem(FileSystem):
sweeet

[cduvall, 2012-07-18 21:26:09]

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/api_data_source_test.py:23: fetcher =
LocalFileSystem(self._base_path)
On 2012/07/18 10:39:15, kalman wrote:
> file_system =
> 
> or inline it.

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/appengine_url_fetcher.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_url_fetcher.py:7: class
AppEngineUrlFetcher(object):
On 2012/07/18 10:39:15, kalman wrote:
> documentation

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_url_fetcher.py:11: def
fetch(self, url):
On 2012/07/18 10:39:15, kalman wrote:
> These should all be Google-style, like Fetch.

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_url_fetcher.py:17: def
make_fetch_call(self, rpc, path):
On 2012/07/18 10:39:15, kalman wrote:
> This *is* the appengine API, I know; but it could be done a higher level
> (particularly if we're going to make fake ones). It's just that this
> create_rpc/make_fetch_call yuck is a pain to fake out.
> 
> Coud you have it be like, Fetch (calls .fetch directly) and FetchAsync
(returns
> a Future for a fetch value and does the create_rpc stuff itself).

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/async_fetch_value.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/async_fetch_value.py:7: class
AsyncFetchValue(object):
On 2012/07/18 10:39:15, kalman wrote:
> Hm, sorry, this is pretty tied into the FileSystem stuff. Specifically
> SubversionFileSystem. Could you move it into there? And... err, rename it
> _AsyncFetchFuture.

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/async_fetch_value.py:38: if self._error is
not None:
On 2012/07/18 10:39:15, kalman wrote:
> what's wrong with !=

From PEP 8: "Comparisons to singletons like None should always be done with is
or is not, never the equality operators."

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/branch_utility.py:27: return channel_name
On 2012/07/18 10:39:15, kalman wrote:
> I posit that this isn't a number
> 
> (just sayin)

It could be something like "1180_38" which isn't really a number either.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/branch_utility.py:29: fetch_data =
urlfetch.fetch(base_path)
On 2012/07/18 10:39:15, kalman wrote:
> The result of this fetch should be cached. Just memcache it for an hour or so.
> 
> Though that'd mean you need to abstract away memcache for testing. Bleh. Maybe
> you'll need to do that anyway to test memcache_file_system.
> 
> Btw please make this a class, injecting urlfetch into every call to
> GetBranchNumberForChannelName is icky. Ditto base_path.

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/branch_utility.py:31: raise
Exception('Fetch returned zero results.')
On 2012/07/18 10:39:15, kalman wrote:
> It'd crash anyway with an invalid JSON thing, so checking here is kinda moot.
I
> don't see why empty is any different to invalid json.

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/branch_utility.py:46: sorted_list = [x for
x in branch_numbers.iteritems()]
On 2012/07/18 10:39:15, kalman wrote:
> x for x in blah... isn't that just blah?
> 
> If you're trying to listify, then list(branch_numbers.iteritems()) would do
it.
> then again, for that matter, so would branch_numbers.items().
> 
> then again, this whole expression can just be:
> sorted_list = sorted(branch_numbers.iteritems(),
>                      key=operator.itemgetter(1),
>                      reversed=True)
> 
> also sorted_list is pretty vague, sorted_branches or something. whatever it's
> supposed to be.

Wow this was ugly. I haven't looked at it since I started working on the docs
server.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/branch_utility.py:47: sorted_list.sort(key
= lambda x: x[1])
On 2012/07/18 10:39:15, kalman wrote:
> key=itemgetter(1)

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/echo_handler.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/echo_handler.py:72: svn_fs =
SubversionFileSystem(fetcher)
On 2012/07/18 10:39:15, kalman wrote:
> inline svn_fs?

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/echo_handler.py:107:
self._GetInstanceForBranch(branch).Get(real_path, self)
On 2012/07/18 10:39:15, kalman wrote:
> musing: when branch versions bump, this leaks Server instances. We might want
to
> add some logic to detect that at some point. Add TODO?

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/example_zipper.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:13: def __init__(self,
fetcher, cache_builder, base_path, match_path):
On 2012/07/18 10:39:15, kalman wrote:
> s/fetcher/file_system/g

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:33: def create(self,
path):
On 2012/07/18 10:39:15, kalman wrote:
> s/create/Create/

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/fake_url_fetcher.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/fake_url_fetcher.py:7: class
FakeUrlFetcher(object):
On 2012/07/18 10:39:15, kalman wrote:
> See comment in appengine_url_fetcher; it would simplify this class a lot.

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/fetcher_cache.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/fetcher_cache.py:8: class
FetcherCache(object):
On 2012/07/18 10:39:15, kalman wrote:
> probably needs a rename/redocument.
> 
> also, s/fetcher/file_system/g

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/fetcher_cache.py:23: class
_CacheEntry(object):
On 2012/07/18 10:39:15, kalman wrote:
> Here's the thing with this class now, by the way: the time-based caching is
> redundant. And kinda of makes it... less good.
> 
> Basically, we should change the way this works, from having time-based cache
> invalidation to querying Stat on the file system for whether it's expired.

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/fetcher_cache.py:51: def getFromFile(self,
key):
On 2012/07/18 10:39:15, kalman wrote:
> document
> 
> and key -> path

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/fetcher_cache.py:62: def
getFromFileListing(self, key):
On 2012/07/18 10:39:15, kalman wrote:
> document
> 
> also key -> path
> 
> btw this method should be tolerant to input that doesn't end in a / ... either
> that or crash if it does. I prefer the former.

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/file_system.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/file_system.py:10: def Read(self, paths):
pass
On 2012/07/18 10:39:15, kalman wrote:
> raise NotImplementedError()
> 
> also newline after the method

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/file_system.py:11: def Stat(self, path):
On 2012/07/18 10:39:15, kalman wrote:
> documentation on all these. and on the classes.

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/file_system.py:12: return 0
On 2012/07/18 10:39:15, kalman wrote:
> raise NotImplementedError()?

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/lazy_value.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/lazy_value.py:7: class LazyValue(object):
On 2012/07/18 10:39:15, kalman wrote:
> I realised today that this isn't *like* Java's Future. It *is* Java's Future.
> 
> So... call it Future. Sorry bout that :)
> 
> also: documentation

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/lazy_value.py:14: self._delegate is None):
On 2012/07/18 10:39:15, kalman wrote:
> what's wrong with ==

See previous comment.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/lazy_value.py:17: def Get(self):
On 2012/07/18 10:39:15, kalman wrote:
> documentation

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/lazy_value.py:20: if self._error is not
None:
On 2012/07/18 10:39:15, kalman wrote:
> what's wrong with !=

See previous comment.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/local_file_system.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/local_file_system.py:11: """Class to fetch
resources from local filesystem.
On 2012/07/18 10:39:15, kalman wrote:
> FileSystem implementation which fetches resources from the local filesystem.

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/memcache_file_system.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:7: class
MemcacheFileSystem(object):
On 2012/07/18 10:39:15, kalman wrote:
> documentation etc. Why does this not extend FileSystem while LocalFileSystem
> does? For documentation's sake this should extend FileSystem, I think.

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:10: self._memcache
= memcache
On 2012/07/18 10:39:15, kalman wrote:
> Class needs tests! And for that you'll probably need to pull out memcache into
> its own little abstraction thing (AppengineMemcache / InMemoryMemcache),
though
> it'd be much more trivial than the urlfetch thing. Very trivial.

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:13: directory =
path.rsplit('/', 1)[0]
On 2012/07/18 10:39:15, kalman wrote:
> Stat should be at the path level, not the directory level. If somebody wants
to
> stat a directory they should convert the path into a directory first.

We can only stat directories, not files. See 3 comments down.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:14: stat_info =
self._memcache.get(directory, namespace='directories')
On 2012/07/18 10:39:15, kalman wrote:
> we should be more disciplined with these namespaces. either name them with a
> stronger convention (e.g. here, 'MemcacheFileSystem.Stat', below,
> 'MemcacheFileSystem.Read'), or list them somewhere.
> 
> Actually what might be nice (and would have prevented those bugs below) is to
> make your memcache abstraction have a compulsory namespace, and have those
> listed. E.g.
> 
> class Memcache(object):
>   MEMCACHE_FILE_SYSTEM_READ = "MemcacheFileSystem.Get"
>   MEMCACHE_FILE_SYSTEM_STAT = "MemcacheFileSystem.Stat"
> 
>   def Get(self, key):
>     raise NotImplementedError()
> 
> class AppengineMemcache(object):
>   def Get(self, key, time=60, namespace=None):
>     if namespace == None:
>       raise ValueError(...)
>     return memcache.get(key, time=time, namespace=namespace)

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:25: if
cached_result is None:
On 2012/07/18 10:39:16, kalman wrote:
> what's wrong with ==

See previous comment.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:29: if
self.Stat(path) > version:
On 2012/07/18 10:39:16, kalman wrote:
> So this means that if any file in a directory changes, all files in that
> directory get invalidated. Hm. If Stat took a path, this logic *could* be
> 
> if (self.Stat(_GetDir(path)) > version and
>     self.Stat(path) > version):
>   ...
> 
> which wouldn't be a whole lot better since now there's twice the number of
stats
> (although they get cached, so, hm, maybe not).
> 
> My point is that this could be *even better* as:
> 
> data, version = cached_result
> if self.Stat(_GetDir(path)) > version:
>   # Some entities(s) in the directory has changed. It might be this one?
>   entity_version = self.Stat(path).version
>   if entity_version > version:
>     # Entity has changed. Invalidate it and read the whole contents back
later.
>     memcache.Delete(path, namespace=...)
>     uncached.append(path)
>     continue
>   # Entity *hasn't* changed, and that means we can bump its version
>   # in memcache so that it doesn't need to be re-stat'ed next time.
>   memcache.Set(path, (data, entity_version), namespace=...)
> result[path] = Future(value=data)
> 
> p.s. try to document these nontrivial algorithms.
> 
> p.s. also my brain is getting a bit sore but this all seems a bit prone to
race
> conditions, if memcache can change underneath us. Something to look into.

The only problem with this: We can't stat files, just directories. The version
is scraped from the HTML of the directory page, and fetching files just returns
contents.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:30:
self._memcache.delete(path)
On 2012/07/18 10:39:16, kalman wrote:
> also missing namespace

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:37:
self._memcache.set(item, (new_items[item], version), namespace='data')
On 2012/07/18 10:39:16, kalman wrote:
> set but never get?
> 
> Oh, the get is missing its namespace. Foundabug!

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:38:
new_items[item] = LazyValue(value=new_items[item])
On 2012/07/18 10:39:16, kalman wrote:
> why not
> result[item] = LazyValue(value=new_items[item])
> ?
> then you don't need the update, and don't need to be writing over the data
> that's being iterated over (which looks strange)

Why not? Because I'm a dummy.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:25: [path +
'/manifest.json'])[path + '/manifest.json'].Get()
On 2012/07/18 10:39:16, kalman wrote:
> seems like you should hold onto path + '/manifest.json' in a variable.

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:39: api_items =
set([])
On 2012/07/18 10:39:16, kalman wrote:
> don't need the [], just set()

Done.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:40:
[self._GetApiItems(api_items, js.Get()) for js in js_contents.values()]
On 2012/07/18 10:39:16, kalman wrote:
> I preferred the way it was... side-effectful list comprehensions doesn't seem
> like a good idiom.

Done.

[not at google - send to devlin, 2012-07-19 03:55:18]

Most of my comments are either fairly cosmetic or should be done in a follow-up,
so lgtm.

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/async_fetch_value.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/async_fetch_value.py:38: if self._error is
not None:
On 2012/07/18 21:26:10, cduvall wrote:
> On 2012/07/18 10:39:15, kalman wrote:
> > what's wrong with !=
> 
> From PEP 8: "Comparisons to singletons like None should always be done with is
> or is not, never the equality operators."

wtf python. "is not" is an operator? That could equally be read as

if self._error is (not None):

which is clearly none. Except apparnetly Python has the world's strangest
operator. They shoud have made it "isnt", they have "elif", why not...

kinda kidding. but also kinda not.

</rant>

or maybe that should be

{{/rant}}

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/memcache_file_system.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:13: directory =
path.rsplit('/', 1)[0]
On 2012/07/18 21:26:10, cduvall wrote:
> On 2012/07/18 10:39:15, kalman wrote:
> > Stat should be at the path level, not the directory level. If somebody wants
> to
> > stat a directory they should convert the path into a directory first.
> 
> We can only stat directories, not files. See 3 comments down.

Ah I see. Bummer, this would be so neat (see comment below) if it did.

We *could* use viewvc for the stat, which gives the version of every file as
well as the directory itself. Then
- if stat'ing a file, returns just the file's version
- if stat'ing a dir, returns the dir's version plus a "files" dict which maps
the file in each directory to its version (part of StatInfo obviously)

So SubversionFileSystem would need to do a bunch of extra HTML parsing for that
(see
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/
for example) and its implementation of Stat for files would be fairly
inefficient: stat the dir, then pull out just the file.

but...

Then MemcacheFileSystem can be smart, something like:

def Stat(self, path):
  cached = memcache.get(path)
  if cached:
    return cached

  is_dir = path.endswith('/')
  if is_dir:
    dir_path = path
  else
    dir_path, file_path = path.rsplit('/', 1)
    dir_path += '/'
  dir_path = path if is_dir else path[:path.rindex('/') + 1]
  dir_stat = file_system.Stat(dir_path)

  memcache.set(dir_path, dir_stat.version)
  for file_name, file_stat in dir_stat.files.iteritems():
    memcache.set(dir_path + '/' + file_name, file_stat)

  return dir_stat if is_dir else dir_stat.files[file_path]

So, I think that would ultimately turn out to be quite a bit more efficient,
since you update all file stats at once and therefore don't need to read back
every file when one gets invalidated.

However -- unless you're really keen to do that now, it's fine to do it in a
follow-up. There is a lot of value in this patch already, and the change would
be incremental. Plus if it turns out to actually *not* be any better, well, easy
to revert :)

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:29: if
self.Stat(path) > version:
On 2012/07/18 21:26:10, cduvall wrote:
> On 2012/07/18 10:39:16, kalman wrote:
> > So this means that if any file in a directory changes, all files in that
> > directory get invalidated. Hm. If Stat took a path, this logic *could* be
> > 
> > if (self.Stat(_GetDir(path)) > version and
> >     self.Stat(path) > version):
> >   ...
> > 
> > which wouldn't be a whole lot better since now there's twice the number of
> stats
> > (although they get cached, so, hm, maybe not).
> > 
> > My point is that this could be *even better* as:
> > 
> > data, version = cached_result
> > if self.Stat(_GetDir(path)) > version:
> >   # Some entities(s) in the directory has changed. It might be this one?
> >   entity_version = self.Stat(path).version
> >   if entity_version > version:
> >     # Entity has changed. Invalidate it and read the whole contents back
> later.
> >     memcache.Delete(path, namespace=...)
> >     uncached.append(path)
> >     continue
> >   # Entity *hasn't* changed, and that means we can bump its version
> >   # in memcache so that it doesn't need to be re-stat'ed next time.
> >   memcache.Set(path, (data, entity_version), namespace=...)
> > result[path] = Future(value=data)
> > 
> > p.s. try to document these nontrivial algorithms.
> > 
> > p.s. also my brain is getting a bit sore but this all seems a bit prone to
> race
> > conditions, if memcache can change underneath us. Something to look into.
> 
> The only problem with this: We can't stat files, just directories. The version
> is scraped from the HTML of the directory page, and fetching files just
returns
> contents.

See comment above.

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/appengine_memcache.py (right):

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/appengine_memcache.py:21:
namespace=self._branch + namespace,
could you put the branch at the end (with a . separating), i think that might
look better when viewing stuff that's memcached.

Which I presume you can do.

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/appengine_url_fetcher.py (right):

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/appengine_url_fetcher.py:29: class
_AsyncFetchDelegate(object):
nit: at top, outside the class scope

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/branch_utility.py:62: return
sorted_branches[0][0]
can you cache this value rather than the unparsed json?

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/echo_handler.py (right):

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/echo_handler.py:77:
AppEngineMemcache(branch, memcache))
so pretty...

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/fake_url_fetcher.py (right):

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/fake_url_fetcher.py:9: class
FakeUrlFetcher(object):
this isn't really a fake since it reads stuff off the file system. doesn't
matter though.

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/fake_url_fetcher.py:32: self.status_code =
200
nit: can go outside the class at the top

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/memcache_file_system_test.py (right):

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:37:
(self._file_system._memcache._cache[memcache.MEMCACHE_FILE_SYSTEM_READ]
Shoudn't need to access local variables like this. That's why we would hold onto
the memcache ourselves locally, in setUp():

self._memcache = InMemoryMemcache()
self._file_system = ..

then use _memcache here.

However, likewise, don't access private members of it. Either add test-friendly
methods to it, or utilise its interface methods; I think the latter would work
fine in this particular case?

expected.remove('file0.html')
self._memcache.Set('list/', expected, memcache.MEMCACHE_FILE_SYSTEM_READ)

etc.

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:43: 
In any case, these tests don't really test the point of MemcacheFileSystem, in
that it's supposed to be efficient.

Like, test that if Stat returns the same thing, Read doesn't get called.

For that you might want more of a Mock object, or at least, an object that
records how many times "read" has been called or something.

But I think you should add those kind of tests after we resolve what the
interface to Stat should be.

[cduvall, 2012-07-19 17:18:28]

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/async_fetch_value.py (right):

http://codereview.chromium.org/10704252/diff/3041/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/async_fetch_value.py:38: if self._error is
not None:
On 2012/07/19 03:55:18, kalman wrote:
> On 2012/07/18 21:26:10, cduvall wrote:
> > On 2012/07/18 10:39:15, kalman wrote:
> > > what's wrong with !=
> > 
> > From PEP 8: "Comparisons to singletons like None should always be done with
is
> > or is not, never the equality operators."
> 
> wtf python. "is not" is an operator? That could equally be read as
> 
> if self._error is (not None):
> 
> which is clearly none. Except apparnetly Python has the world's strangest
> operator. They shoud have made it "isnt", they have "elif", why not...
> 
> kinda kidding. but also kinda not.
> 
> </rant>
> 
> or maybe that should be
> 
> {{/rant}}

haha {{/rant}}

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/appengine_memcache.py (right):

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/appengine_memcache.py:21:
namespace=self._branch + namespace,
On 2012/07/19 03:55:19, kalman wrote:
> could you put the branch at the end (with a . separating), i think that might
> look better when viewing stuff that's memcached.
> 
> Which I presume you can do.

Done.

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/appengine_url_fetcher.py (right):

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/appengine_url_fetcher.py:29: class
_AsyncFetchDelegate(object):
On 2012/07/19 03:55:19, kalman wrote:
> nit: at top, outside the class scope

Done.

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/branch_utility.py:62: return
sorted_branches[0][0]
On 2012/07/19 03:55:19, kalman wrote:
> can you cache this value rather than the unparsed json?

Done.

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/fake_url_fetcher.py (right):

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/fake_url_fetcher.py:32: self.status_code =
200
On 2012/07/19 03:55:19, kalman wrote:
> nit: can go outside the class at the top

Done.

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/memcache_file_system_test.py (right):

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:37:
(self._file_system._memcache._cache[memcache.MEMCACHE_FILE_SYSTEM_READ]
On 2012/07/19 03:55:19, kalman wrote:
> Shoudn't need to access local variables like this. That's why we would hold
onto
> the memcache ourselves locally, in setUp():
> 
> self._memcache = InMemoryMemcache()
> self._file_system = ..
> 
> then use _memcache here.
> 
> However, likewise, don't access private members of it. Either add
test-friendly
> methods to it, or utilise its interface methods; I think the latter would work
> fine in this particular case?
> 
> expected.remove('file0.html')
> self._memcache.Set('list/', expected, memcache.MEMCACHE_FILE_SYSTEM_READ)
> 
> etc.

Done.

http://codereview.chromium.org/10704252/diff/18076/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:43: 
On 2012/07/19 03:55:19, kalman wrote:
> In any case, these tests don't really test the point of MemcacheFileSystem, in
> that it's supposed to be efficient.
> 
> Like, test that if Stat returns the same thing, Read doesn't get called.
> 
> For that you might want more of a Mock object, or at least, an object that
> records how many times "read" has been called or something.
> 
> But I think you should add those kind of tests after we resolve what the
> interface to Stat should be.

Ok, sounds good.