Docserver: Add support for viewing docs with a codereview patch applied

This change would make it easier for docs reviewers to preview patched docs
without locally patching in or asking patch author to stage it on a
workstation. Patched docs will be available at
https://developer.chrome.com/_patch/issue_number/extensions/... after
this CL is landed and deployed.

Note that this doesn't work for patches that involve server-side changes.

R=kalman@chromium.org
BUG=234022
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199727

[方觉(Fang Jue), 2013-04-26 13:19:38]

It's not quite finished but should probably work. Besides 14159018, I also
tested http://crrev.com/14259005, http://crrev.com/13116014 and
http://crrev.com/11785039. Unfortunately, Datastore Write quota nearly ran out.
(So I don't continue testing static (js/css) changes, intro changes, removing
files, etc.)

I'm trying to reuse existing caches for trunk as much as possible and have
separate caches (memcache) for every different patch (identified with issue
number and patchset number). Samples are always unpatched as it consumes a lot
of resources and it's easy to imagine what a new sample item will look like. I
didn't simply disable samples because we might want to polish samples page
later.

Just took a glance at app engine dashboard and realized that static changes
won't work. We'll have to make {{static}} point to patched static paths.

Also, I'll need to write unit tests (and it's the first time for me to write
Python tests:).

[not at google - send to devlin, 2013-04-26 23:53:53]

Some preliminary comments.

For tests, look at how some of the newer tests are written.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/app.yaml:10: upload: robots.txt
cheers, should steal a favicon too I suppose, and delete those lines where it's
hardcoded.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/handler.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/handler.py:44: channel_name, issue,
real_path = (
We only need to support 'trunk' here. Note for future.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/handler.py:47: channel_name, real_path =
BranchUtility.SplitChannelNameFromPath(path)
I've just written https://codereview.chromium.org/14273035/ to make this all a
more pluggable architecture. It's still a bit more intertwined than I'd like,
but that patch should be enough to get started with.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/object_store_creator.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/object_store_creator.py:24:
patchset=None):
I reckon we should make this stuff more generic, like have an "more" argument to
specify the issue and patchset. ObjectStoreCreator is too general a class to
worry about implementation specifies like that.

I.e. here

def __init__(self, ..., more=None):
  ...
  self._more = more

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/object_store_creator.py:76: '' if issue is
None else '.%s.%s' % (issue, patchset))
... (continuing from above) then have

if self._more:
  self._name = '%s/%s' % (self._name, '/'.join('%s=%s' % k, self._more[k]
                          for k in sorted(self._more.iterkeys()))

so that you'd get something like

'2-0-23/RietveldFileSystem@trunk/issue=2387419/patchset=3947'

honestly, I'd be tempted to just make it *all* very generic and just have a
generic "components" dictionary. Make this:

self._components = {
  'version': version,
  'branch': branch,
  'cls': cls.__name__,
}
self._components.update(more)

and the whole thing would be

'version=2-0-23/cls=RietveldFileSystem/branch=trunk/issue=2387419/patchset=3947'

well actually it would be that, except the keys would be sorted.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/patched_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/patched_file_system.py:18:
unpatched_paths.append(path)
you may want to use set()s everywhere for these patched paths rather than lists.
It will make this kind of logic much simpler. See set.difference etc.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/patched_file_system.py:22:
self._patched_future = patch_file_system.Read(patched_paths, binary)
Just pass these futures directly in through the constructor.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/rietveld_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:12:
RIETVELD_ISSUE_TARBALL = 'tarball/%s/%s'
constants with hanging substitutions are odd... they're not really constants.
Just inline that.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:13:
RIETVELD_TARBALL_PATCHED_FILE = 'b/' + EXTENSIONS_PATH + '/%s'
ditto. and string concatentation is slow, except perhaps in the case of only
catt-ing two strings. Anyway, use %s.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:24: raise
FileNotFoundError('Failed to download tarball.')
this means we'll be unpacking the tarball every time something from the
filesystem is read. Instead we should cache the result as soon as it's read and
unpacked, then always use that (cache on RietveldFileSystem obviously).

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:43: return value
.. this is the value to cache. In fact, most of the time Read would actually
return a Future(value=...) with the value that is being created here.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:47: """
Use ' not ".

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:48: def
__init__(self, issue, patchset, patched_files, fetcher):
pass through that PatchData object that you'll write rather than
issue/patchset/patched_files.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:67: return
StatInfo('0', child_versions)
I can't actually really read this, besides, the Stat for these files is probably
the patchset.

Implement something like:

if path.endswith('/'):
  return StatInfo(
      patchset,
      child_versions=dict((child, patchset) for child in patched_files
                          if child.startswith(path))
return StatInfo(patchset)

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/rietveld_utility.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_utility.py:32: class
RietveldUtility(object):
I think much of this class will transform nicely into PatchServlet when you
write it.

Generally speaking util classes are undesirable. It's not clear how they relate
to the rest of the code.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:104: issue is None
I don't think "issue is None" means anything as a statement, maybe you mean
"issue = None".

However, this is the kind of data that should happen outside this class so that
the error can be sensibly dealt with. Perhaps bundle up the issue and patchset
(and any other data) into a patch_data class, pass that in here.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:116: issue, patchset)
patched files... changes... everything should be passed into this class rather
than determined by it.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:120: True, #
start_empty
use start_empty=True

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:122: patchset)
it's nice to spell out optional arguments explicitly, though depending on how
ObjectStoreCreator turns out, this may be inevitable.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:140: svn_file_system,
I hope that it's possible to inject patched_file_system in place of
svn_file_system here and everything will work. I'm surprised there is so much
complication here.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:166: return
ServerInstance.rietveld_utility
this would probably end up getting owned by PatchServlet?

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:187: def __init__(self,
As above - I think most of __init__ can stay the same, you just need to inject
that patched file system instead of the svn file system

[not at google - send to devlin, 2013-04-26 23:54:30]

Incidentally, looking at your demo was so incredibly cool. I can't wait for this
to go live.

[方觉(Fang Jue), 2013-04-27 00:24:34]

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/handler.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/handler.py:44: channel_name, issue,
real_path = (
On 2013/04/26 23:53:53, kalman wrote:
> We only need to support 'trunk' here. Note for future.
Yes. I know that. And SplitPatchFromPath will always return 'trunk' here.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/handler.py:47: channel_name, real_path =
BranchUtility.SplitChannelNameFromPath(path)
On 2013/04/26 23:53:53, kalman wrote:
> I've just written https://codereview.chromium.org/14273035/ to make this all a
> more pluggable architecture. It's still a bit more intertwined than I'd like,
> but that patch should be enough to get started with.

I did take a look at it just now before I had breakfast. I'll update my patch
when that one is landed.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/rietveld_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:24: raise
FileNotFoundError('Failed to download tarball.')
On 2013/04/26 23:53:53, kalman wrote:
> this means we'll be unpacking the tarball every time something from the
> filesystem is read. Instead we should cache the result as soon as it's read
and
> unpacked, then always use that (cache on RietveldFileSystem obviously).

There's cache on RietveldFileSystem (memcache, see server_instance.py), but no
cache for the tarball (and probably multiple requests are made for the same
tarball). Maybe also cache tarball in memcache (what if it's too large (although
it should occasionally happen))?

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:67: return
StatInfo('0', child_versions)
On 2013/04/26 23:53:53, kalman wrote:
> I can't actually really read this, besides, the Stat for these files is
probably
> the patchset.
> 
> Implement something like:
> 
> if path.endswith('/'):
>   return StatInfo(
>       patchset,
>       child_versions=dict((child, patchset) for child in patched_files
>                           if child.startswith(path))
> return StatInfo(patchset)

Different patchsets in an issue get different namespaces for memcache so they
won't conflict with each other. A patchset, once created, should not change.
That's why I return '0' as version for all files here.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:140: svn_file_system,
On 2013/04/26 23:53:53, kalman wrote:
> I hope that it's possible to inject patched_file_system in place of
> svn_file_system here and everything will work. I'm surprised there is so much
> complication here.

I was trying to reuse existing cached content for trunk. That's why there's
complication here and below.
I'm sure replacing svn_file_system with patched_file_system will work. But we'll
end up having the same data cached in different namespaces and reconstructing
everything (whether changed in the patch or not) for different patches.
What do you think?

[not at google - send to devlin, 2013-04-27 00:45:30]

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/handler.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/handler.py:44: channel_name, issue,
real_path = (
On 2013/04/27 00:24:34, 方觉(Fang Jue) wrote:
> On 2013/04/26 23:53:53, kalman wrote:
> > We only need to support 'trunk' here. Note for future.
> Yes. I know that. And SplitPatchFromPath will always return 'trunk' here.

Yep, as in when you pull this into its own servlet there won't need to be that
redundant channel_name variable.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/rietveld_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:24: raise
FileNotFoundError('Failed to download tarball.')
On 2013/04/27 00:24:34, 方觉(Fang Jue) wrote:
> On 2013/04/26 23:53:53, kalman wrote:
> > this means we'll be unpacking the tarball every time something from the
> > filesystem is read. Instead we should cache the result as soon as it's read
> and
> > unpacked, then always use that (cache on RietveldFileSystem obviously).
> 
> There's cache on RietveldFileSystem (memcache, see server_instance.py), but no
> cache for the tarball (and probably multiple requests are made for the same
> tarball). Maybe also cache tarball in memcache (what if it's too large
(although
> it should occasionally happen))?

There is a cache, though it's still the case that two different files will be
cached separately, so requesting "extensions/foo.html" and later
"extensions/bar.html" will do another fetch of the tarball.

TBH it's quite unlikely that these will be large, so I'd check them all into an
ObjectStore (the individual files). Using a CachingFileSystem around it becomes
unnecessary then, but that's fine, a lot of the smart caching stuff it does is
moot here anyway since the data doesn't change.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:67: return
StatInfo('0', child_versions)
On 2013/04/27 00:24:34, 方觉(Fang Jue) wrote:
> On 2013/04/26 23:53:53, kalman wrote:
> > I can't actually really read this, besides, the Stat for these files is
> probably
> > the patchset.
> > 
> > Implement something like:
> > 
> > if path.endswith('/'):
> >   return StatInfo(
> >       patchset,
> >       child_versions=dict((child, patchset) for child in patched_files
> >                           if child.startswith(path))
> > return StatInfo(patchset)
> 
> Different patchsets in an issue get different namespaces for memcache so they
> won't conflict with each other. A patchset, once created, should not change.
> That's why I return '0' as version for all files here.

Ok, that makes sense. Let's see how it turns out eventually. This approach *is*
simpler but it's also akin to mirroring subversion at every version - it's
potentially wasteful. The number of patchsets uploaded to these things is
hopefully fairly low, but it could number in the 10s and that's 10x more data
than we need.

What I'm saying is that maybe we *will* end up implementing these on a per-issue
basis rather than a per-issue per-patchset basis. In fact, what do you think;
how much extra work would that be?

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:140: svn_file_system,
On 2013/04/27 00:24:34, 方觉(Fang Jue) wrote:
> On 2013/04/26 23:53:53, kalman wrote:
> > I hope that it's possible to inject patched_file_system in place of
> > svn_file_system here and everything will work. I'm surprised there is so
much
> > complication here.
> 
> I was trying to reuse existing cached content for trunk. That's why there's
> complication here and below.
> I'm sure replacing svn_file_system with patched_file_system will work. But
we'll
> end up having the same data cached in different namespaces and reconstructing
> everything (whether changed in the patch or not) for different patches.
> What do you think?

patched_file_system delegates to svn_file_system, which is a CachingFileSystem,
so you'll still get all the caching, or am I misunderstanding?

[方觉(Fang Jue), 2013-04-27 01:02:48]

(new patchset is uploading for comments replied with 'Done.')

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/object_store_creator.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/object_store_creator.py:24:
patchset=None):
On 2013/04/26 23:53:53, kalman wrote:
> I reckon we should make this stuff more generic, like have an "more" argument
to
> specify the issue and patchset. ObjectStoreCreator is too general a class to
> worry about implementation specifies like that.
> 
> I.e. here
> 
> def __init__(self, ..., more=None):
>   ...
>   self._more = more

Done.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/object_store_creator.py:76: '' if issue is
None else '.%s.%s' % (issue, patchset))
On 2013/04/26 23:53:53, kalman wrote:
> ... (continuing from above) then have
> 
> if self._more:
>   self._name = '%s/%s' % (self._name, '/'.join('%s=%s' % k, self._more[k]
>                           for k in sorted(self._more.iterkeys()))
> 
> so that you'd get something like
> 
> '2-0-23/RietveldFileSystem@trunk/issue=2387419/patchset=3947'
> 
> honestly, I'd be tempted to just make it *all* very generic and just have a
> generic "components" dictionary. Make this:
> 
> self._components = {
>   'version': version,
>   'branch': branch,
>   'cls': cls.__name__,
> }
> self._components.update(more)
> 
> and the whole thing would be
> 
>
'version=2-0-23/cls=RietveldFileSystem/branch=trunk/issue=2387419/patchset=3947'
> 
> well actually it would be that, except the keys would be sorted.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/patched_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/patched_file_system.py:18:
unpatched_paths.append(path)
On 2013/04/26 23:53:53, kalman wrote:
> you may want to use set()s everywhere for these patched paths rather than
lists.
> It will make this kind of logic much simpler. See set.difference etc.

Done. Python is awesome!

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/patched_file_system.py:22:
self._patched_future = patch_file_system.Read(patched_paths, binary)
On 2013/04/26 23:53:53, kalman wrote:
> Just pass these futures directly in through the constructor.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/rietveld_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:12:
RIETVELD_ISSUE_TARBALL = 'tarball/%s/%s'
On 2013/04/26 23:53:53, kalman wrote:
> constants with hanging substitutions are odd... they're not really constants.
> Just inline that.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:13:
RIETVELD_TARBALL_PATCHED_FILE = 'b/' + EXTENSIONS_PATH + '/%s'
On 2013/04/26 23:53:53, kalman wrote:
> ditto. and string concatentation is slow, except perhaps in the case of only
> catt-ing two strings. Anyway, use %s.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:47: """
On 2013/04/26 23:53:53, kalman wrote:
> Use ' not ".

Done.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:67: return
StatInfo('0', child_versions)
On 2013/04/27 00:45:30, kalman wrote:
> On 2013/04/27 00:24:34, 方觉(Fang Jue) wrote:
> > On 2013/04/26 23:53:53, kalman wrote:
> > > I can't actually really read this, besides, the Stat for these files is
> > probably
> > > the patchset.
> > > 
> > > Implement something like:
> > > 
> > > if path.endswith('/'):
> > >   return StatInfo(
> > >       patchset,
> > >       child_versions=dict((child, patchset) for child in patched_files
> > >                           if child.startswith(path))
> > > return StatInfo(patchset)
> > 
> > Different patchsets in an issue get different namespaces for memcache so
they
> > won't conflict with each other. A patchset, once created, should not change.
> > That's why I return '0' as version for all files here.
> 
> Ok, that makes sense. Let's see how it turns out eventually. This approach
*is*
> simpler but it's also akin to mirroring subversion at every version - it's
> potentially wasteful. The number of patchsets uploaded to these things is
> hopefully fairly low, but it could number in the 10s and that's 10x more data
> than we need.
> 
> What I'm saying is that maybe we *will* end up implementing these on a
per-issue
> basis rather than a per-issue per-patchset basis. In fact, what do you think;
> how much extra work would that be?

But these are memcached (and are never intended to be persisted). Cached data
for older patchsets will be automatically purged eventually.
Implementing RietveldFileSystem on a per-issue basis means that it has to make
(extra? synchronous) requests to get patchset number in order to construct
tarball URL.
But maybe it makes more sense. Different patchsets in an issue are similar to
different revisions in svn. If we decide not to reuse CompiledFileSystem caches,
this should be the right thing to do.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:140: svn_file_system,
On 2013/04/27 00:45:30, kalman wrote:
> On 2013/04/27 00:24:34, 方觉(Fang Jue) wrote:
> > On 2013/04/26 23:53:53, kalman wrote:
> > > I hope that it's possible to inject patched_file_system in place of
> > > svn_file_system here and everything will work. I'm surprised there is so
> much
> > > complication here.
> > 
> > I was trying to reuse existing cached content for trunk. That's why there's
> > complication here and below.
> > I'm sure replacing svn_file_system with patched_file_system will work. But
> we'll
> > end up having the same data cached in different namespaces and
reconstructing
> > everything (whether changed in the patch or not) for different patches.
> > What do you think?
> 
> patched_file_system delegates to svn_file_system, which is a
CachingFileSystem,
> so you'll still get all the caching, or am I misunderstanding?

SubversionFileSystem will get all the caching but what about CompiledFileSystem?

[not at google - send to devlin, 2013-04-27 01:07:48]

FYI I'm about to head home for the weekend.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/rietveld_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:67: return
StatInfo('0', child_versions)
> Different patchsets in an issue are similar to
> different revisions in svn.

Yeah, exactly.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:140: svn_file_system,
On 2013/04/27 01:02:48, 方觉(Fang Jue) wrote:
> On 2013/04/27 00:45:30, kalman wrote:
> > On 2013/04/27 00:24:34, 方觉(Fang Jue) wrote:
> > > On 2013/04/26 23:53:53, kalman wrote:
> > > > I hope that it's possible to inject patched_file_system in place of
> > > > svn_file_system here and everything will work. I'm surprised there is so
> > much
> > > > complication here.
> > > 
> > > I was trying to reuse existing cached content for trunk. That's why
there's
> > > complication here and below.
> > > I'm sure replacing svn_file_system with patched_file_system will work. But
> > we'll
> > > end up having the same data cached in different namespaces and
> reconstructing
> > > everything (whether changed in the patch or not) for different patches.
> > > What do you think?
> > 
> > patched_file_system delegates to svn_file_system, which is a
> CachingFileSystem,
> > so you'll still get all the caching, or am I misunderstanding?
> 
> SubversionFileSystem will get all the caching but what about
CompiledFileSystem?

Ah I see what you mean. Eh, re-compiling all that stuff isn't a big deal.

[方觉(Fang Jue), 2013-04-29 12:46:23]

Patch updated and deployed. Use https://chrome-apps-doc-test-2.appspot.com if
you get quota errors.
Tests for patched_file_system and rietveld_file_system are also included but
they haven't yet cover everything.

Patch to /static works now (example:
https://chrome-apps-doc-test.appspot.com/_patch/13824011/apps/about_apps.html ,
choose a branch and you'll see a message box) by adding a branch_path argument
to TemplateDataSource.

Everything originally in rietveld_utility has been moved to patch_servlet. When
you land your patch, I'll update it to be a real servlet accordingly. I also
made a comment there.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/app.yaml:10: upload: robots.txt
On 2013/04/26 23:53:53, kalman wrote:
> cheers, should steal a favicon too I suppose, and delete those lines where
it's
> hardcoded.

Done. But you need to land binary files for me.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/object_store_creator.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/object_store_creator.py:76: '' if issue is
None else '.%s.%s' % (issue, patchset))
On 2013/04/27 01:02:48, 方觉(Fang Jue) wrote:
> On 2013/04/26 23:53:53, kalman wrote:
> > ... (continuing from above) then have
> > 
> > if self._more:
> >   self._name = '%s/%s' % (self._name, '/'.join('%s=%s' % k, self._more[k]
> >                           for k in sorted(self._more.iterkeys()))
> > 
> > so that you'd get something like
> > 
> > '2-0-23/RietveldFileSystem@trunk/issue=2387419/patchset=3947'
> > 
> > honestly, I'd be tempted to just make it *all* very generic and just have a
> > generic "components" dictionary. Make this:
> > 
> > self._components = {
> >   'version': version,
> >   'branch': branch,
> >   'cls': cls.__name__,
> > }
> > self._components.update(more)
> > 
> > and the whole thing would be
> > 
> >
>
'version=2-0-23/cls=RietveldFileSystem/branch=trunk/issue=2387419/patchset=3947'
> > 
> > well actually it would be that, except the keys would be sorted.
> 
> Done.

In the latest patchset, the namespace will be something like:
branch=test/cls=CompiledFileSystem/version=3-0/id/file
Is that a little too long? If that's ok, I'll update the corresponding strings
in *_test.py.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/rietveld_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:24: raise
FileNotFoundError('Failed to download tarball.')
On 2013/04/27 00:45:30, kalman wrote:
> On 2013/04/27 00:24:34, 方觉(Fang Jue) wrote:
> > On 2013/04/26 23:53:53, kalman wrote:
> > > this means we'll be unpacking the tarball every time something from the
> > > filesystem is read. Instead we should cache the result as soon as it's
read
> > and
> > > unpacked, then always use that (cache on RietveldFileSystem obviously).
> > 
> > There's cache on RietveldFileSystem (memcache, see server_instance.py), but
no
> > cache for the tarball (and probably multiple requests are made for the same
> > tarball). Maybe also cache tarball in memcache (what if it's too large
> (although
> > it should occasionally happen))?
> 
> There is a cache, though it's still the case that two different files will be
> cached separately, so requesting "extensions/foo.html" and later
> "extensions/bar.html" will do another fetch of the tarball.
> 
> TBH it's quite unlikely that these will be large, so I'd check them all into
an
> ObjectStore (the individual files). Using a CachingFileSystem around it
becomes
> unnecessary then, but that's fine, a lot of the smart caching stuff it does is
> moot here anyway since the data doesn't change.

Done.
Now RietveldFileSystem has built-in cache. However, I have to get values from
cache synchronously in Read in order to determine whether fetching the tarball
is needed.
When a tarball is fetched, it will extract all files that are not yet cached and
then cache them.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:48: def
__init__(self, issue, patchset, patched_files, fetcher):
On 2013/04/26 23:53:53, kalman wrote:
> pass through that PatchData object that you'll write rather than
> issue/patchset/patched_files.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/rietveld_file_system.py:67: return
StatInfo('0', child_versions)
On 2013/04/27 01:07:48, kalman wrote:
> > Different patchsets in an issue are similar to
> > different revisions in svn.
> 
> Yeah, exactly.

Now, RietveldFileSystem is no longer wrapped by CachingFileSystem. It's only
wrapped by PatchedFileSystem, which never calls Stat on it. So I removed Stat
here.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:120: True, #
start_empty
On 2013/04/26 23:53:53, kalman wrote:
> use start_empty=True

Done.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:122: patchset)
On 2013/04/26 23:53:53, kalman wrote:
> it's nice to spell out optional arguments explicitly, though depending on how
> ObjectStoreCreator turns out, this may be inevitable.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:140: svn_file_system,
On 2013/04/27 01:07:48, kalman wrote:
> On 2013/04/27 01:02:48, 方觉(Fang Jue) wrote:
> > On 2013/04/27 00:45:30, kalman wrote:
> > > On 2013/04/27 00:24:34, 方觉(Fang Jue) wrote:
> > > > On 2013/04/26 23:53:53, kalman wrote:
> > > > > I hope that it's possible to inject patched_file_system in place of
> > > > > svn_file_system here and everything will work. I'm surprised there is
so
> > > much
> > > > > complication here.
> > > > 
> > > > I was trying to reuse existing cached content for trunk. That's why
> there's
> > > > complication here and below.
> > > > I'm sure replacing svn_file_system with patched_file_system will work.
But
> > > we'll
> > > > end up having the same data cached in different namespaces and
> > reconstructing
> > > > everything (whether changed in the patch or not) for different patches.
> > > > What do you think?
> > > 
> > > patched_file_system delegates to svn_file_system, which is a
> > CachingFileSystem,
> > > so you'll still get all the caching, or am I misunderstanding?
> > 
> > SubversionFileSystem will get all the caching but what about
> CompiledFileSystem?
> 
> Ah I see what you mean. Eh, re-compiling all that stuff isn't a big deal.

Then complicated and messy changes below are no longer necessary!

https://chromiumcodereview.appspot.com/14125010/diff/9001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:187: def __init__(self,
On 2013/04/26 23:53:53, kalman wrote:
> As above - I think most of __init__ can stay the same, you just need to inject
> that patched file system instead of the svn file system

Done.

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
chrome/common/extensions/docs/server2/app.yaml:13: upload: favicon.ico
Please upload a favicon.ico before this patch is landed.

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patched_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:28: # TODO(fj):
What about directory listing?
I found that directory listing is used in APIListDataSource (and probably
somewhere else). If a file for an API is removed, it still appears in chrome.*
API page (for example,
https://chrome-apps-doc-test.appspot.com/_patch/14096030/extensions/api_index...
. runtime.html was removed in that patch.). It isn't a big problem though.

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
File chrome/common/extensions/docs/server2/robots.txt (right):

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
chrome/common/extensions/docs/server2/robots.txt:2: Disallow: /_cron/
By the way, should we also require admin login for cron jobs?

[not at google - send to devlin, 2013-04-29 16:38:49]

One last thing - if you don't mind the occasional rebase you can base this patch
off the servlet one, then when you upload specify that as a base.

Using git cherry-pick can be a nice way to maintain this workflow if you squash
down commits to a single patch against that version.

Give me another few hours to fix up the servlet patch, though.

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patched_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:28: # TODO(fj):
What about directory listing?
On 2013/04/29 12:46:23, 方觉(Fang Jue) wrote:
> I found that directory listing is used in APIListDataSource (and probably
> somewhere else). If a file for an API is removed, it still appears in chrome.*
> API page (for example,
>
https://chrome-apps-doc-test.appspot.com/_patch/14096030/extensions/api_index...
> . runtime.html was removed in that patch.). It isn't a big problem though.

Do you mean normally? As in the server right now, if a file is removed then it
still shows up in the API list? Or is this something that is specific to the
patched file system (I presume the former).

We should at least fix that at some point.

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:62: # are all '0'.
I was thinking about this more and we shouldn't be re-creating these for each
patchset that gets uploaded.

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patched_file_system_test.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system_test.py:54: 
See comment in PatchedFileSystem. If we can remove the Rietveld knowledge from
there the test in here can become much more unit-testable (e.g. see
CachingFileSystem for a good example).

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
File chrome/common/extensions/docs/server2/rietveld_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_file_system.py:115:
self._object_store))
I'm confused what the role of PatchedFileSystem vs RietveldFileSystem is. By its
name PatchedFileSystem seems like a generic class which applies some patch to
the output of an existing file system then serves it. However, it has Rietveld
specific knowledge. This class obviously also has Rietveld knowledge and it
seems the division is semi arbitrary.

Here is an architecture that I'd prefer: and as a preface, I think that we need
to do as we discussed earlier and not re-create these for each patchset. It
feels a bit off the way it's happening at the moment.

PatchedFileSystem - like I mention above, take the Rietveld knowledge out of it.
It takes a file system and some way of patching files, then serves those patched
files. The "way of patching files" is just a simple interface which takes a path
and file contents, then returns new file contents.

class Patcher(object):
  class Patch(object):
    def Apply(self, file_contents):
      '''Returns |file_contents| with its contents patched
      against |self|.
      '''
      raise NotImplemented()
    def GetVersion(self):
      raise NotImplemented()

  def GetPatches(self, paths):
    '''Returns a Future Patch for each paths.
    Implemented this way so that we can do parallel fetches.
    '''
    raise NotImplemented()
  def GetPatch(self, path):
    # implemented against GetPatches

class PatchedFileSystem(FileSystem):
  def __init__(self, file_system, patcher):
    self._file_system = file_system
    self._patcher = patcher

  def Read(self, paths):
    # equivalent to:
    return self._patcher.GetPatches(paths).Apply(
        self.file_system.Read(paths))

  def Stat(self, path):
    # equivalent to:
    return self._file_system.Stat(path) +
           self._patcher.GetPatch(path).GetVersion())
    # though + won't quite work obviously.

then the main implementation of FilePatcher becomes RietveldPatcher (with all of
the logic you've already written) and we can write a TestPatcher for testing.

Sorry to keep on turning you around like this. Once I submit that servlet patch
I think it'll become clearer what the architecture should be.

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
File chrome/common/extensions/docs/server2/robots.txt (right):

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
chrome/common/extensions/docs/server2/robots.txt:2: Disallow: /_cron/
On 2013/04/29 12:46:23, 方觉(Fang Jue) wrote:
> By the way, should we also require admin login for cron jobs?

Good point. It looks like we can either do this in the app.yaml or check for
X-Apppengine-Cron header ourselves:

https://developers.google.com/appengine/docs/python/config/cron#Securing_URLs...

The former makes more sense, either way, separate patch...

[not at google - send to devlin, 2013-04-29 16:56:21]

class Patcher(object):
  class Patch(object):
    def Apply(self, file_contents):
      '''Returns |file_contents| with its contents patched
      against |self|.
      '''
      raise NotImplemented()
    def GetVersion(self):
      raise NotImplemented()

I just remembered - the "patching" is actually just getting the new version from
rietveld, there is no actual patching required? That would make the
implementation of this class pretty trivial. Might as well un-interface it.

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
File chrome/common/extensions/docs/server2/rietveld_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_file_system.py:58:
patched_file.close()
you may be able to do

with tar.extractfile(tar_path) as patched_file:
  data = patched_file.read()

(no close())

[方觉(Fang Jue), 2013-04-30 01:07:11]

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patched_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:28: # TODO(fj):
What about directory listing?
On 2013/04/29 16:38:49, kalman wrote:
> On 2013/04/29 12:46:23, 方觉(Fang Jue) wrote:
> > I found that directory listing is used in APIListDataSource (and probably
> > somewhere else). If a file for an API is removed, it still appears in
chrome.*
> > API page (for example,
> >
>
https://chrome-apps-doc-test.appspot.com/_patch/14096030/extensions/api_index...
> > . runtime.html was removed in that patch.). It isn't a big problem though.
> 
> Do you mean normally? As in the server right now, if a file is removed then it
> still shows up in the API list? Or is this something that is specific to the
> patched file system (I presume the former).
> 
> We should at least fix that at some point.

It's specific to the patched file system. When fetching directory listing, the
path ends with '/'. No path in patched_files will end with '/'. Therefore it
will eventually directly call svn_file_system's Read.

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
File chrome/common/extensions/docs/server2/rietveld_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_file_system.py:115:
self._object_store))
On 2013/04/29 16:38:49, kalman wrote:
> I'm confused what the role of PatchedFileSystem vs RietveldFileSystem is. By
its
> name PatchedFileSystem seems like a generic class which applies some patch to
> the output of an existing file system then serves it. However, it has Rietveld
> specific knowledge. This class obviously also has Rietveld knowledge and it
> seems the division is semi arbitrary.
> 
> Here is an architecture that I'd prefer: and as a preface, I think that we
need
> to do as we discussed earlier and not re-create these for each patchset. It
> feels a bit off the way it's happening at the moment.
> 
> PatchedFileSystem - like I mention above, take the Rietveld knowledge out of
it.
> It takes a file system and some way of patching files, then serves those
patched
> files. The "way of patching files" is just a simple interface which takes a
path
> and file contents, then returns new file contents.
> 
> class Patcher(object):
>   class Patch(object):
>     def Apply(self, file_contents):
>       '''Returns |file_contents| with its contents patched
>       against |self|.
>       '''
>       raise NotImplemented()
>     def GetVersion(self):
>       raise NotImplemented()
> 
>   def GetPatches(self, paths):
>     '''Returns a Future Patch for each paths.
>     Implemented this way so that we can do parallel fetches.
>     '''
>     raise NotImplemented()
>   def GetPatch(self, path):
>     # implemented against GetPatches
> 
> class PatchedFileSystem(FileSystem):
>   def __init__(self, file_system, patcher):
>     self._file_system = file_system
>     self._patcher = patcher
> 
>   def Read(self, paths):
>     # equivalent to:
>     return self._patcher.GetPatches(paths).Apply(
>         self.file_system.Read(paths))
> 
>   def Stat(self, path):
>     # equivalent to:
>     return self._file_system.Stat(path) +
>            self._patcher.GetPatch(path).GetVersion())
>     # though + won't quite work obviously.
> 
> then the main implementation of FilePatcher becomes RietveldPatcher (with all
of
> the logic you've already written) and we can write a TestPatcher for testing.
> 
> Sorry to keep on turning you around like this. Once I submit that servlet
patch
> I think it'll become clearer what the architecture should be.

Perhaps just combine RietveldFileSystem and PatchedFileSystem into one
PatchedFileSystem?

[not at google - send to devlin, 2013-04-30 04:21:48]

On 2013/04/30 01:07:11, 方觉(Fang Jue) wrote:
>
https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
> File chrome/common/extensions/docs/server2/patched_file_system.py (right):
> 
>
https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
> chrome/common/extensions/docs/server2/patched_file_system.py:28: # TODO(fj):
> What about directory listing?
> On 2013/04/29 16:38:49, kalman wrote:
> > On 2013/04/29 12:46:23, 方觉(Fang Jue) wrote:
> > > I found that directory listing is used in APIListDataSource (and probably
> > > somewhere else). If a file for an API is removed, it still appears in
> chrome.*
> > > API page (for example,
> > >
> >
>
https://chrome-apps-doc-test.appspot.com/_patch/14096030/extensions/api_index...
> > > . runtime.html was removed in that patch.). It isn't a big problem though.
> > 
> > Do you mean normally? As in the server right now, if a file is removed then
it
> > still shows up in the API list? Or is this something that is specific to the
> > patched file system (I presume the former).
> > 
> > We should at least fix that at some point.
> 
> It's specific to the patched file system. When fetching directory listing, the
> path ends with '/'. No path in patched_files will end with '/'. Therefore it
> will eventually directly call svn_file_system's Read.
> 
>
https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
> File chrome/common/extensions/docs/server2/rietveld_file_system.py (right):
> 
>
https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
> chrome/common/extensions/docs/server2/rietveld_file_system.py:115:
> self._object_store))
> On 2013/04/29 16:38:49, kalman wrote:
> > I'm confused what the role of PatchedFileSystem vs RietveldFileSystem is. By
> its
> > name PatchedFileSystem seems like a generic class which applies some patch
to
> > the output of an existing file system then serves it. However, it has
Rietveld
> > specific knowledge. This class obviously also has Rietveld knowledge and it
> > seems the division is semi arbitrary.
> > 
> > Here is an architecture that I'd prefer: and as a preface, I think that we
> need
> > to do as we discussed earlier and not re-create these for each patchset. It
> > feels a bit off the way it's happening at the moment.
> > 
> > PatchedFileSystem - like I mention above, take the Rietveld knowledge out of
> it.
> > It takes a file system and some way of patching files, then serves those
> patched
> > files. The "way of patching files" is just a simple interface which takes a
> path
> > and file contents, then returns new file contents.
> > 
> > class Patcher(object):
> >   class Patch(object):
> >     def Apply(self, file_contents):
> >       '''Returns |file_contents| with its contents patched
> >       against |self|.
> >       '''
> >       raise NotImplemented()
> >     def GetVersion(self):
> >       raise NotImplemented()
> > 
> >   def GetPatches(self, paths):
> >     '''Returns a Future Patch for each paths.
> >     Implemented this way so that we can do parallel fetches.
> >     '''
> >     raise NotImplemented()
> >   def GetPatch(self, path):
> >     # implemented against GetPatches
> > 
> > class PatchedFileSystem(FileSystem):
> >   def __init__(self, file_system, patcher):
> >     self._file_system = file_system
> >     self._patcher = patcher
> > 
> >   def Read(self, paths):
> >     # equivalent to:
> >     return self._patcher.GetPatches(paths).Apply(
> >         self.file_system.Read(paths))
> > 
> >   def Stat(self, path):
> >     # equivalent to:
> >     return self._file_system.Stat(path) +
> >            self._patcher.GetPatch(path).GetVersion())
> >     # though + won't quite work obviously.
> > 
> > then the main implementation of FilePatcher becomes RietveldPatcher (with
all
> of
> > the logic you've already written) and we can write a TestPatcher for
testing.
> > 
> > Sorry to keep on turning you around like this. Once I submit that servlet
> patch
> > I think it'll become clearer what the architecture should be.
> 
> Perhaps just combine RietveldFileSystem and PatchedFileSystem into one
> PatchedFileSystem?

That would be fine. The way above might make unit testing easier and help break
up a pretty complex class though, in a way that makes sense to me.

[方觉(Fang Jue), 2013-04-30 14:25:08]

Now PatchedFileSystem takes a file system and a patcher. The interface of
Patcher is different from what you proposed, however. And Stat should be
correctly patched now with version='patched_<patchset number>'.

There're still other things to do but it's time to sleep. I'm still not sure
whether fetching patch data should stay in RietveldPatcher (when Stat/Read) or
Handler (or PatchServlet in the future) (before handling a request). Also the
way to let files in different patchsets have separate cache is too tricky (and
why ObjectStoreCreator's category can't contain digits?)

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patched_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:62: # are all '0'.
On 2013/04/29 16:38:49, kalman wrote:
> I was thinking about this more and we shouldn't be re-creating these for each
> patchset that gets uploaded.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
File chrome/common/extensions/docs/server2/rietveld_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_file_system.py:58:
patched_file.close()
On 2013/04/29 16:56:21, kalman wrote:
> you may be able to do
> 
> with tar.extractfile(tar_path) as patched_file:
>   data = patched_file.read()
> 
> (no close())

It ended up with AttributeError: __exit__.

[not at google - send to devlin, 2013-04-30 15:37:42]

This is looking great. I will get the servlet patch committed today so that you
can rebase that.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
File chrome/common/extensions/docs/server2/object_store_creator.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/object_store_creator.py:24:
persistent=True,
I was wondering, it actually seems fine to just persist this. At some point I'll
write code to clear out stale object store entries, and besides we have a lot of
object store quota, so that's fine. It means this stuff will end up being
faster.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/object_store_creator.py:96: assert not
any(c.isdigit() for c in category)
to answer your question - the reason why category can't have number is so that
it doesn't get confused with version. However, you're changing the way this
works anyway, so you can remove this restriction.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/object_store_creator.py:102: object_stores
+= (PersistentObjectStore(namespace),)
fwiw, if you're modifying lists, use a list not a tuple.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patched_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:25: dirs_value =
self._svn_dirs_future.Get()
maybe call these files/dirs rather than generic value/dirs_value.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:31: class
Patcher(object):
I think it would be nicer to have this in a separate file, it's pretty meaty -
and the idea was to have Patcher be as dumb as possible so that unit testing
PatchedFileSystemis as meaningful as possible. There's quite a lot of logic in
here.

I mean - I guess it's ok, you'd just need to separately unit-test Patcher.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:33: def
GetPatchedFiles(self):
comments should be like:

def GetPatchedFiles(self):
  '''Returns (added_files, deleted_files, modified_files).
  '''

same for others.

And please document all of the other public methods here.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:49: if
f.startswith(path):
something like:

result = [f[len(path):] for f in files if f.startswith(path)]

except I don't understand the "if '/' in f: f = f.split('/, 1)[0] + '/'" line.

I presume this whole method is suppose to list the patched files underneath
|path|? Perhaps GetPatchedFilesInDir would be better, call |path| |dir| instead.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:57:
tuple(_FindChildrenInPath(files) for files in self.GetPatchedFiles()))
this method doesn't appear to return anything..?

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:59: def
PatchDirectoryListing(self, path, value):
s/value/original_dir_listing/ or similar.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:64: version =
self.GetVersion()
when would GetVersion return None? can we just guarantee that it's never None?

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:67: version =
'patched_' + version
This kind of logic is FileSystem specific, doesn't belong in Patcher. I looks
like the PatchDirectoryListing/PatchStat methods belong in PatchedFileSystem.

Incidentally, for more complex string building (e.g really anything beyond
appending a '/') it's nice to use 'patched_%s' % version.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:71: 
It's a bit unclear to me how the logic below relates to patching a stat. It's a
bit indirect with conditions that aren't clear - e.g. like I mentioned, it seems
like stat_info can be None right at the bottom if this existing_children thing
is empty.

I would imagine it might look something more like:

if path in deleted:
  raise FileNotFoundException(...)

if path in added:
  assert stat_info is None
  if path.endswith('/'):
    child_versions = ...
    return StatInfo(version, child_versions=child_versions)
  return StatInfo(version)

assert path in modified
... etc

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:73: if stat_info is
None and len(existing_children) != 0:
how can existing_children be non-empty if the path is added in the patch? 

Actually, perhaps this is my misconception of what "existing_children" means,
since it's assigned to (added + modified) - it seems like it's actually the
children *after* the patch has been applied, children_after_patch or something?

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:76: {child: version
for child in existing_children})
see comment at the end of this method. might as well just return the StatInfo
directly from here.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:78: filename =
path.rsplit('/', 1)[-1]
perhaps assign this at the same time you assign directory i.e.

filename, directory = path.rsplit('/', 1)

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:83: if
len(existing_children) > 0:
you're using > 0 here and != 0 above - nice to be consistent. > 0 is probably
more idiomatic.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:88:
stat_info.version = version
It looks like stat_info can be None here, if existing_children is empty?

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:90: return
stat_info
indentation looks incorrect here, the "if stat_info is None and
len(existing_children != 0" never returns a value.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:102: patched_files
+= files
patched_files = set(self._patcher.GetPatchedFiles())?

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:103: dir_paths =
[path for path in paths if path.endswith('/')]
assign directly into a set here? I suspect that file_system.Read works with a
set as well as a list, and so does the _AsyncFetchFuture stuff.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:105: patched_paths
= list(set(file_paths) & set(patched_files))
looks like you're converting from a set to a list here, then in both times it's
used back into a set.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:117: except
FileNotFoundError as e:
The unfortunate part of writing it like this is that Python is silly and doesn't
preserve stack traces when you re-throw errors - and stack traces *really* help
when trying to debug failures.

So, try to write this in such a way that this call ends up falling through to a
FileNotFoundError - e.g. first check whether this file is being added, and if
it's not then grab the stat (let FileNotFoundError fall through).

It would actually make sense to me to move the logic in PatchStat into here, as
I mentioned.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patched_file_system_test.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system_test.py:20: 
Like I was alluding to when I was saying "unit test plz", this test should have
the format:

class PatchedFileSystemTest(unittest.TestCase):
  def setUp(self):
    svn_file_system = FakeFileSystem({
      ...
    })
    patcher = FakePatcher({
      ... you will need to write FakePatcher.
    })
    self._patched_file_system = PatchedFileSystem(svn_file_system, patcher)

then... cover all the cases. For both Stat and Read, test
- modifying existing file
- adding a new file
- remove a file
- querying a file that is in neither file system
particularly for Stat, removing both directories and files.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
File chrome/common/extensions/docs/server2/rietveld_patcher.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:16:
_FILE_NOT_FOUND_VALUE = (None,)
i usually use _FILE_NOT_FOUND_VALUE = object(), and then to check whether
something is that value use "is" not ==.

That said, why do you need this rather than throwing FileNotFoundErrors at the
time it's assigned? I don't see much difference.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:59: print 'Fetching
tarball/%s/%s' % (issue, patchset)
use logging.info. I think it's a useful log to have, at least for now.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:65: raise
FileNotFoundError('Failed to download tarball.')
include status code?

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:70: raise
FileNotFoundError('Invalid tarball.')
include some data about the exception?

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:84:
patched_file.close()
in lieu of the "with..." thing, patched_file.close() should go in a finally:
block after the exception.

also - what exactly can fail here? the extractfile or the read(), or both? how
can they fail? should one of the failures be converted into a FileNotFoundError?
write a brief comment for people like me who shouldn't need to dig up the python
library reference.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:114: class
RietveldPatcher(Patcher):
We need a RietveldPatcherTest, which should use the fake data you've checked in.

(and as I mentioned, PatchedFileSystemTest should just set up fake stuff, not
rely on that rietveld data since it's more abstract than that)

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:130: return
self._GetPatchset()
right - I perhaps make this

return self._GetPatchset() or "0"

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:134: patchset =
self._object_store.Get(key).Get()
this is what category= is for in the Create() method of
object_store_creator_factory.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:160: key = '@%s.%s' %
(self._issue, patchset)
We need a consistent scheme for this - it looks like you're using just the issue
above, but the issue + patchset here. Maybe they're two separate object stores,
see comment above.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:217:
self._object_store_creator_factory.Create(RietveldPatcher).
What is this for? I don't quite understand, but it looks like a misuse of
category. If files change then delete or update them in object store, though
you'd need to keep a list around or something.

Anyway, as a general rule, only Create these in the constructor.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/server_instance.py:111: branch,
this shouldn't actually use the branch, we're only using trunk here.

and I think some of the namespacing problems in RietveldFileSystem could be
solved by passing in the issue number as the branch here.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/server_instance.py:114: more={'issue':
issue})
and I guess you wouldn't need issue:issue here - hm, and that also means that
the keying by issue that I saw happening in RietveldPatcher shouldn't be
necessary?

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/template_data_source.py:69:
self._static_resources = '%s/static' % branch_path
let's just pass in the static path directly rather than using the rather
implementation-detail-y branch_path to figure it out.

[not at google - send to devlin, 2013-04-30 15:39:39]

> There're still other things to do but it's time to sleep. I'm still not sure
> whether fetching patch data should stay in RietveldPatcher (when Stat/Read) or
> Handler (or PatchServlet in the future) (before handling a request).

Fetching patches depends on a specific implementation of Patcher - namely that
they're fetched - so it belongs in RietveldPatcher.

[not at google - send to devlin, 2013-05-01 06:17:00]

Ok - servlet patch has been submitted now. Looking forward to this patch within
there!

[方觉(Fang Jue), 2013-05-01 15:27:25]

Thanks for your patience! Sorry for my clumsy Python.

I have one question for Servlet. In handler.py we have servlet(servlet_path,
request.headers).Get(), but Servlet's constructor takes |request|. How does
servlet(servlet_path, request.headers) become Servlet(Request(servlet_path,
request.headers))?

And another question. How does BranchUtility get updated (when a new Chrome
version comes)? I created an object store for issue's latest patchset number in
a similar way in rietveld_patcher.py but couldn't figure out how to keep it
up-to-date.

It's very late here. Tests will be added tomorrow.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
File chrome/common/extensions/docs/server2/object_store_creator.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/object_store_creator.py:24:
persistent=True,
On 2013/04/30 15:37:42, kalman wrote:
> I was wondering, it actually seems fine to just persist this. At some point
I'll
> write code to clear out stale object store entries, and besides we have a lot
of
> object store quota, so that's fine. It means this stuff will end up being
> faster.

Since you say it's fine, then I'll use persistent object store by default. And
in the end this file doesn't need changing at all!

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patched_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:25: dirs_value =
self._svn_dirs_future.Get()
On 2013/04/30 15:37:42, kalman wrote:
> maybe call these files/dirs rather than generic value/dirs_value.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:33: def
GetPatchedFiles(self):
On 2013/04/30 15:37:42, kalman wrote:
> comments should be like:
> 
> def GetPatchedFiles(self):
>   '''Returns (added_files, deleted_files, modified_files).
>   '''
> 
> same for others.
> 
> And please document all of the other public methods here.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:49: if
f.startswith(path):
On 2013/04/30 15:37:42, kalman wrote:
> something like:
> 
> result = [f[len(path):] for f in files if f.startswith(path)]
> 
> except I don't understand the "if '/' in f: f = f.split('/, 1)[0] + '/'" line.
> 
> I presume this whole method is suppose to list the patched files underneath
> |path|? Perhaps GetPatchedFilesInDir would be better, call |path| |dir|
instead.

Suppose a patch contains:
c/c/e/docs/templates/public/apps/manifest/name.html
c/c/e/docs/templates/public/apps/runtime.html
...
When calling GetPatchedFilesInDir('c/c/e/docs/templates/public/apps/'),
it should return ['manifest/', 'runtime.html', ...] instead of
'manifest/name.html'.

It turned out that dir is a built-in function.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:57:
tuple(_FindChildrenInPath(files) for files in self.GetPatchedFiles()))
On 2013/04/30 15:37:42, kalman wrote:
> this method doesn't appear to return anything..?

oops.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:59: def
PatchDirectoryListing(self, path, value):
On 2013/04/30 15:37:42, kalman wrote:
> s/value/original_dir_listing/ or similar.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:64: version =
self.GetVersion()
On 2013/04/30 15:37:42, kalman wrote:
> when would GetVersion return None? can we just guarantee that it's never None?

When it fails to fetch issue data from Rietveld or when the patch doesn't
contain changes to extension docs.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:67: version =
'patched_' + version
On 2013/04/30 15:37:42, kalman wrote:
> This kind of logic is FileSystem specific, doesn't belong in Patcher. I looks
> like the PatchDirectoryListing/PatchStat methods belong in PatchedFileSystem.
> 
It seems that only the three NotImplemented methods should belong to Patcher.
The other three are specifically used by FileSystem to patch directory listing
and stat, which are FileSystem's implementation details.

> Incidentally, for more complex string building (e.g really anything beyond
> appending a '/') it's nice to use 'patched_%s' % version.
Sorry about that.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:102: patched_files
+= files
On 2013/04/30 15:37:42, kalman wrote:
> patched_files = set(self._patcher.GetPatchedFiles())?

It doesn't work.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:103: dir_paths =
[path for path in paths if path.endswith('/')]
On 2013/04/30 15:37:42, kalman wrote:
> assign directly into a set here? I suspect that file_system.Read works with a
> set as well as a list, and so does the _AsyncFetchFuture stuff.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:105: patched_paths
= list(set(file_paths) & set(patched_files))
On 2013/04/30 15:37:42, kalman wrote:
> looks like you're converting from a set to a list here, then in both times
it's
> used back into a set.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:117: except
FileNotFoundError as e:
On 2013/04/30 15:37:42, kalman wrote:
> The unfortunate part of writing it like this is that Python is silly and
doesn't
> preserve stack traces when you re-throw errors - and stack traces *really*
help
> when trying to debug failures.
> 
> So, try to write this in such a way that this call ends up falling through to
a
> FileNotFoundError - e.g. first check whether this file is being added, and if
> it's not then grab the stat (let FileNotFoundError fall through).
> 
> It would actually make sense to me to move the logic in PatchStat into here,
as
> I mentioned.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
File chrome/common/extensions/docs/server2/rietveld_patcher.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:16:
_FILE_NOT_FOUND_VALUE = (None,)
On 2013/04/30 15:37:42, kalman wrote:
> i usually use _FILE_NOT_FOUND_VALUE = object(), and then to check whether
> something is that value use "is" not ==.
> 
> That said, why do you need this rather than throwing FileNotFoundErrors at the
> time it's assigned? I don't see much difference.

Well, object() is not object() and object() != object(). (None,) == (None,) but
(None,) is not (None,).

See the comment in the latest patchset for the reason.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:59: print 'Fetching
tarball/%s/%s' % (issue, patchset)
On 2013/04/30 15:37:42, kalman wrote:
> use logging.info. I think it's a useful log to have, at least for now.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:65: raise
FileNotFoundError('Failed to download tarball.')
On 2013/04/30 15:37:42, kalman wrote:
> include status code?

Done.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:70: raise
FileNotFoundError('Invalid tarball.')
On 2013/04/30 15:37:42, kalman wrote:
> include some data about the exception?

Done.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:84:
patched_file.close()
On 2013/04/30 15:37:42, kalman wrote:
> in lieu of the "with..." thing, patched_file.close() should go in a finally:
> block after the exception.
> 
> also - what exactly can fail here? the extractfile or the read(), or both? how
> can they fail? should one of the failures be converted into a
FileNotFoundError?
> write a brief comment for people like me who shouldn't need to dig up the
python
> library reference.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:130: return
self._GetPatchset()
On 2013/04/30 15:37:42, kalman wrote:
> right - I perhaps make this
> 
> return self._GetPatchset() or "0"

No. PatchedFileSystem interprets GetVersion() is None as nothing to patch. I
added a comment there.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:160: key = '@%s.%s' %
(self._issue, patchset)
On 2013/04/30 15:37:42, kalman wrote:
> We need a consistent scheme for this - it looks like you're using just the
issue
> above, but the issue + patchset here. Maybe they're two separate object
stores,
> see comment above.

In the latest patchset, GetPatchset uses an object store created in a way
similar to BranchUtility.
GetPatchedFiles's cache is shared with files' data.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:217:
self._object_store_creator_factory.Create(RietveldPatcher).
On 2013/04/30 15:37:42, kalman wrote:
> What is this for? I don't quite understand, but it looks like a misuse of
> category. If files change then delete or update them in object store, though
> you'd need to keep a list around or something.
> 
> Anyway, as a general rule, only Create these in the constructor.

Yes. It's too weird. I decide not to create separate object store for different
patchset. Instead, include patchset in keys for patched files' data so that data
from different patchsets won't conflict with each other.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/server_instance.py:111: branch,
On 2013/04/30 15:37:42, kalman wrote:
> this shouldn't actually use the branch, we're only using trunk here.
> 
> and I think some of the namespacing problems in RietveldFileSystem could be
> solved by passing in the issue number as the branch here.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/server_instance.py:114: more={'issue':
issue})
On 2013/04/30 15:37:42, kalman wrote:
> and I guess you wouldn't need issue:issue here - hm, and that also means that
> the keying by issue that I saw happening in RietveldPatcher shouldn't be
> necessary?

Done.

[not at google - send to devlin, 2013-05-02 00:47:13]

Just replying to your overall comments here, haven't had time to get to this
today:


> Thanks for your patience! Sorry for my clumsy Python.
> 
> I have one question for Servlet. In handler.py we have servlet(servlet_path,
> request.headers).Get(), but Servlet's constructor takes |request|. How does
> servlet(servlet_path, request.headers) become Servlet(Request(servlet_path,
> request.headers))?

Yeah, my bad. Fixed it.

> 
> And another question. How does BranchUtility get updated (when a new Chrome
> version comes)? I created an object store for issue's latest patchset number
in
> a similar way in rietveld_patcher.py but couldn't figure out how to keep it
> up-to-date.
> 

Hm ok yeah this is a bug. I think I'll change the way that we do ObjectStores.
The idea is that in a cron the data will refresh the object stores, while the
instances will read out of persistence - this is how the caching file systems
work, but you're right, the logic isn't transferring into the classes that just
use the plain object stores.

I'll have a look at it.

[方觉(Fang Jue), 2013-05-02 15:29:16]

I was wondering if PatchedFileSystem would work with
ServerInstance.GetOrCreateOffline. If Read and Stat works well when it's
offline, it should work. But I was getting the following error on the live
server:
23.367087495806372771/compiled_file_system.py", line 73, in GetFromFile
    version = self._file_system.Stat(path).version
  File
"/base/data/home/apps/s~chrome-apps-doc-test-2/2-0-23.367087495806372771/patched_file_system.py",
line 136, in Stat
    modified)
  File
"/base/data/home/apps/s~chrome-apps-doc-test-2/2-0-23.367087495806372771/patched_file_system.py",
line 101, in _PatchStat
    stat_info.child_versions[child] = version
TypeError: 'NoneType' object does not support item assignment
Probably there's still something wrong with my implementation of Stat. I have to
look into it tomorrow.

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
File chrome/common/extensions/docs/server2/robots.txt (right):

https://chromiumcodereview.appspot.com/14125010/diff/32001/chrome/common/exte...
chrome/common/extensions/docs/server2/robots.txt:2: Disallow: /_cron/
On 2013/04/29 16:38:49, kalman wrote:
> On 2013/04/29 12:46:23, 方觉(Fang Jue) wrote:
> > By the way, should we also require admin login for cron jobs?
> 
> Good point. It looks like we can either do this in the app.yaml or check for
> X-Apppengine-Cron header ourselves:
> 
>
https://developers.google.com/appengine/docs/python/config/cron#Securing_URLs...
> 
> The former makes more sense, either way, separate patch...

- url: /_cron/.*
  script: appengine_mail.py
  login: admin
before url: .* handler
will simply do it!

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patched_file_system_test.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system_test.py:20: 
On 2013/04/30 15:37:42, kalman wrote:
> Like I was alluding to when I was saying "unit test plz", this test should
have
> the format:
> 
> class PatchedFileSystemTest(unittest.TestCase):
>   def setUp(self):
>     svn_file_system = FakeFileSystem({
>       ...
>     })
>     patcher = FakePatcher({
>       ... you will need to write FakePatcher.
>     })
>     self._patched_file_system = PatchedFileSystem(svn_file_system, patcher)
> 
> then... cover all the cases. For both Stat and Read, test
> - modifying existing file
> - adding a new file
> - remove a file
> - querying a file that is in neither file system
> particularly for Stat, removing both directories and files.

Tests have been improved. Many cases are covered now (but not all I believe).
Unfortunately, SubversionFileSystemTest doesn't provide good Stat data. I
fetched them into test_data/file_system_stat and wrote a FakeStatFetcher.

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/43001/chrome/common/exte...
chrome/common/extensions/docs/server2/template_data_source.py:69:
self._static_resources = '%s/static' % branch_path
On 2013/04/30 15:37:42, kalman wrote:
> let's just pass in the static path directly rather than using the rather
> implementation-detail-y branch_path to figure it out.

I renamed it to base_path (and make it end with '/'). It will also be useful in
https://chromiumcodereview.appspot.com/12521030/.

[not at google - send to devlin, 2013-05-02 15:44:44]

Ok - ping me when the errors are fixed and I'll review. Or should I do it now?
I'm working on fixing that object store problem, then I have some rather urgent
C++ to write.

[方觉(Fang Jue), 2013-05-03 06:16:51]

I just confirmed that PatchedFileSystem will work when offline. Then we can take
advantage of both cached data and cached stat without making any requests to
svn.
Then, PatchServlet would need to access _ALWAYS_ONLINE and create ServerInstance
accordingly.

The errors I encountered last night was due to the difference between
SubversionFileSystem.Stat and CachingFileSystem.Stat. CachingFileSystem.Stat
returns StatInfo(file_version) when stating a file while svn returns the whole
stat info. I'm not sure which one is more appropriate or whether it's a bug. If
we decide to make CachingFileSystem.Stat behave in the same way as svn, I'll
also need to update caching_file_system_test.py.

Today's dev channel update exposed the problem I mentioned earlier (eg. look at
declarativeWebRequest.html). I remember there was a five minute cache for branch
data some time ago but this is not the case now. Anyway, RietveldPatcher cannot
update this information while running cron. Therefore, I implemented a simple
five-minute cache expiration for _GetPatchset.

[方觉(Fang Jue), 2013-05-03 15:30:24]

I modified PatchedFileSystem to make it work when stat_info.child_versions is
None instead of modifying CachingFileSystem and corresponding test expectations.
It is up to you to decide whether this behavior of CachingFileSystem is desired.

I fixed a subtle bug that only reproduced when offline. CacheChainObjectStore
has in memory caches. However, objects are not copied in assignments in Python
(like JavaScript). When I patch stat_info, the cached data is also patched,
resulting in 404 for changes involving api/*. Hopefully, I finally recognized it
and fixed it.

Then everything works well when offline (and it's faster than online). Hopefully
we're closer.

And should the _ALWAYS_ONLINE thing now belong to ServerInstance (and determine
CreateOnline/Offline in ServerInstance)?

Also I'd like to point out something that might be a problem. Every time the
server is deployed (even if version in app.yaml doesn't change), GetAppVersion()
will change because GAE appends something like '.366939230490104064' after the
version. This means that in the first few minutes after a new version is
deployed, caches are not yet filled and normal requests will fail. (at least
this is the case when I deploy to chrome-apps-doc-test-2.)

> At some point I'll write code to clear out stale object store entries
Just a note: You probably need to first add separate properties for branch, app
version, class name, etc. to make it possible to query entries to delete. GQL
doesn't support LIKE queries.

[not at google - send to devlin, 2013-05-03 16:19:33]

On 2013/05/03 06:16:51, 方觉(Fang Jue) wrote:
> I just confirmed that PatchedFileSystem will work when offline. Then we can
take
> advantage of both cached data and cached stat without making any requests to
> svn.
> Then, PatchServlet would need to access _ALWAYS_ONLINE and create
ServerInstance
> accordingly.

How does it work when offline; the cronjobs don't run over the patches?

> 
> The errors I encountered last night was due to the difference between
> SubversionFileSystem.Stat and CachingFileSystem.Stat. CachingFileSystem.Stat
> returns StatInfo(file_version) when stating a file while svn returns the whole
> stat info. I'm not sure which one is more appropriate or whether it's a bug.
If
> we decide to make CachingFileSystem.Stat behave in the same way as svn, I'll
> also need to update caching_file_system_test.py.

Yeah everything should be consistent. SVN returns the whole stat info... of
what? Hm, it looks like there's a bug in there where the child_versions don't
get cleared off it, https://codereview.chromium.org/14759009 should fix it.

> 
> Today's dev channel update exposed the problem I mentioned earlier (eg. look
at
> declarativeWebRequest.html). I remember there was a five minute cache for
branch
> data some time ago but this is not the case now.

Yes I removed it in favour of only updating the data in cronjobs, for stability
and speed. That patch you commented on earlier will fix it once I figure out a
better way to do it.

Hopefully soon before somebody notices the dev channel thing.

> Anyway, RietveldPatcher cannot
> update this information while running cron. Therefore, I implemented a simple
> five-minute cache expiration for _GetPatchset.

Ok cool, that answers my earlier question. I think we may want more like an
instantaneous timeout for GetPatchset, it would be ideal if the patch updates
immediately. Does this have much affect on performance?

[not at google - send to devlin, 2013-05-03 16:28:06]

On 2013/05/03 15:30:24, 方觉(Fang Jue) wrote:
> I modified PatchedFileSystem to make it work when stat_info.child_versions is
> None instead of modifying CachingFileSystem and corresponding test
expectations.
> It is up to you to decide whether this behavior of CachingFileSystem is
desired.
> 
> I fixed a subtle bug that only reproduced when offline. CacheChainObjectStore
> has in memory caches. However, objects are not copied in assignments in Python
> (like JavaScript). When I patch stat_info, the cached data is also patched,
> resulting in 404 for changes involving api/*. Hopefully, I finally recognized
it
> and fixed it.

Yep I'm aware of copy by reference in python, you should deepcopy data that you
intend to modify. I'll have a look at the code.

> 
> Then everything works well when offline (and it's faster than online).
Hopefully
> we're closer.
> 
> And should the _ALWAYS_ONLINE thing now belong to ServerInstance (and
determine
> CreateOnline/Offline in ServerInstance)?

I'm going to delete that asap.

> 
> Also I'd like to point out something that might be a problem. Every time the
> server is deployed (even if version in app.yaml doesn't change),
GetAppVersion()
> will change because GAE appends something like '.366939230490104064' after the
> version. This means that in the first few minutes after a new version is
> deployed, caches are not yet filled and normal requests will fail. (at least
> this is the case when I deploy to chrome-apps-doc-test-2.)

Ah, thank you. I'll need to strip that off.

> 
> > At some point I'll write code to clear out stale object store entries
> Just a note: You probably need to first add separate properties for branch,
app
> version, class name, etc. to make it possible to query entries to delete. GQL
> doesn't support LIKE queries.

Thanks.

Going to start looking at code now.

[not at google - send to devlin, 2013-05-03 19:12:22]

This is looking fantastic. Hopefully the tail end of the comments. In the
meantime I'll get that refactoring patch thing in.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patch_servlet.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patch_servlet.py:15: path_with_issue =
self._request.path.lstrip('/')
'/' is already been lstripped in Handler.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patch_servlet.py:20: real_path = ''
I like the pattern of doing "issue, real_path = (path_with_issue, '')" here,
it's a nice parallel to line 17.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patch_servlet.py:22: return
Response.Redirect('/' + path_with_issue)
Why does the issue need to be a number? We show a 404 when the issue isn't found
right? In which case, this can follow the same eventual path.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patch_servlet.py:23: elif len(real_path)
== 0:
these can be if not elif since the preceding statements are returns.

That said, I would hope that they're not necessary, the redirects are an
implementation detail of RenderServlet.Get passing it the real path. If we
inspect the result of RenderServlet and see that it's a redirect then prepend
the _patch/%s stuff to the path and redirect to that (i.e. update the Location
header). Makes sense to add an IsRedirect method to Response.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patch_servlet.py:30: constructor =
(ServerInstance.CreateOnline if _ALWAYS_ONLINE else
ALWAYS_ONLINE is about to be deleted, use IsDevServer directly.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patched_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:24: self._patcher =
patcher
don't seem to be using this

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:31: dirs[path])
why is this on a line by itself?

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:56: raise
NotImplementedError()
pls put this in a different file.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:61: def
__init__(self, svn_file_system, patcher):
I'm going to start calling svn_file_system 'host_file_system' since 'svn' is too
much of an implementation detail.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:103: stat_info =
deepcopy(stat_info)
yep, thanks

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patched_file_system_test.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system_test.py:22: return '1'
ditto not used

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system_test.py:77:
ConfigureFakeFetchers(os.path.join(sys.path[0], os.pardir))
it should be possible to write this test using purely fake file systems. I.e.
use a TestFileSystem here rather than calling out to the filesystem (... or at
least a LocalFileSystem if for some reason it's necessary to look at files).

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
File chrome/common/extensions/docs/server2/rietveld_patcher.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:17:
_FILE_NOT_FOUND_VALUE = (None,)
This is a bit messy, can you store the deleted files in a separate set?

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:43: return result
use a list comprehension

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:44: else:
previous statement is a return, don't need an else:

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:51: return result
comprehension here too

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:54: result = {}
and here

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:92: 
It would actually be nice to show a more informative page than a 404, such as
"patchset not found", maybe raise a different exception to catch in
PatchServlet. Can do that in a follow-up though.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:101: if
self._cached_value.get(path) is not None:
"path in self._cached_value"?

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:161:
RietveldPatcher).Create()
For the _FILE_NOT_FOUND_VALUE stuff,

self._modified_object_store = oscf.Create(RP).Create(category='modified')
self._removed_object_store = ...('removed')
self._added_object_store = ...('added')

it seems like trying to put them in the same place is causing a bit of
unnecessary complication, particularly with the removed (not sure about added).
it would be easier to reason about if they were separated.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:163: def
GetVersion(self):
No longer used, can delete this.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:170: if datetime.now()
- time < _ISSUE_CACHE_MAXAGE:
We should implement the caching with a CachingPatcher that delegates to another
one. That will make this file targeted to just fetching to rietveld, and
implementation details like caching elsewhere.

Another thing that would let you do - clear that cache on every request that
goes into PatchServlet. I don't think that would greatly hurt performance, and
it would be more useful of a feature if the patch updated immediately.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:196: empty = ([], [],
[])
having this as a variable is a bit less readable than inlining IMO

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:197: patched_files =
self._object_store.Get(object_store_key).Get()
you could also store the list of files in a separate object store. Implementing
these categories via key conventions can get out of control and hard to follow.

In fact I'm finding it hard to follow already, have like a
patched_file_object_store or somethign.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/server_instance.py:51: def
GetOrCreateOffline(channel, base_path=None, issue=None):
After https://codereview.chromium.org/14856006 hopefully all changes to this
file will be unnecessary - you'll pass in a custom host_file_system to be the
PatchedFileSystem.

My mostly-finished SVN pinning patch https://codereview.chromium.org/14247024/
does something similar with CronServlet, though it doesn't quite work yet.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/template_data_source.py:69:
self._static_resources = '%sstatic' % base_path
Pass static resources through rather than base_path?

[方觉(Fang Jue), 2013-05-04 02:05:41]

It is a good idea to separate caching logic from RietveldPatcher.
And using fake file system in patched_file_system_test is definitely better.
I'll do these next.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patch_servlet.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patch_servlet.py:15: path_with_issue =
self._request.path.lstrip('/')
On 2013/05/03 19:12:22, kalman wrote:
> '/' is already been lstripped in Handler.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patch_servlet.py:20: real_path = ''
On 2013/05/03 19:12:22, kalman wrote:
> I like the pattern of doing "issue, real_path = (path_with_issue, '')" here,
> it's a nice parallel to line 17.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patch_servlet.py:20: real_path = ''
On 2013/05/03 19:12:22, kalman wrote:
> I like the pattern of doing "issue, real_path = (path_with_issue, '')" here,
> it's a nice parallel to line 17.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patch_servlet.py:22: return
Response.Redirect('/' + path_with_issue)
On 2013/05/03 19:12:22, kalman wrote:
> Why does the issue need to be a number? We show a 404 when the issue isn't
found
> right? In which case, this can follow the same eventual path.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patch_servlet.py:23: elif len(real_path)
== 0:
On 2013/05/03 19:12:22, kalman wrote:
> these can be if not elif since the preceding statements are returns.
> 
> That said, I would hope that they're not necessary, the redirects are an
> implementation detail of RenderServlet.Get passing it the real path. If we
> inspect the result of RenderServlet and see that it's a redirect then prepend
> the _patch/%s stuff to the path and redirect to that (i.e. update the Location
> header). Makes sense to add an IsRedirect method to Response.

Done.
When I tested it, I found a bug in RenderServlet.Get. It doesn't correctly
handle paths like 'trunk/extension' (without trailing slash).

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patch_servlet.py:30: constructor =
(ServerInstance.CreateOnline if _ALWAYS_ONLINE else
On 2013/05/03 19:12:22, kalman wrote:
> ALWAYS_ONLINE is about to be deleted, use IsDevServer directly.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patched_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:24: self._patcher =
patcher
On 2013/05/03 19:12:22, kalman wrote:
> don't seem to be using this

Done.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:31: dirs[path])
On 2013/05/03 19:12:22, kalman wrote:
> why is this on a line by itself?

Done.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:56: raise
NotImplementedError()
On 2013/05/03 19:12:22, kalman wrote:
> pls put this in a different file.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system.py:61: def
__init__(self, svn_file_system, patcher):
On 2013/05/03 19:12:22, kalman wrote:
> I'm going to start calling svn_file_system 'host_file_system' since 'svn' is
too
> much of an implementation detail.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patched_file_system_test.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system_test.py:22: return '1'
On 2013/05/03 19:12:22, kalman wrote:
> ditto not used

But GetVersion must be implemented.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
File chrome/common/extensions/docs/server2/rietveld_patcher.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:43: return result
On 2013/05/03 19:12:22, kalman wrote:
> use a list comprehension

Done.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:44: else:
On 2013/05/03 19:12:22, kalman wrote:
> previous statement is a return, don't need an else:

Done.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:51: return result
On 2013/05/03 19:12:22, kalman wrote:
> comprehension here too

Done.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:54: result = {}
On 2013/05/03 19:12:22, kalman wrote:
> and here

Done.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:101: if
self._cached_value.get(path) is not None:
On 2013/05/03 19:12:22, kalman wrote:
> "path in self._cached_value"?

Done.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:170: if datetime.now()
- time < _ISSUE_CACHE_MAXAGE:
> Another thing that would let you do - clear that cache on every request that
> goes into PatchServlet. I don't think that would greatly hurt performance, and
> it would be more useful of a feature if the patch updated immediately.

Maybe GetPatchset without cache when constructing RietveldPatcher? List of files
in a patchset and their content cannot change after a patchset is uploaded.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:196: empty = ([], [],
[])
On 2013/05/03 19:12:22, kalman wrote:
> having this as a variable is a bit less readable than inlining IMO

Done.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/template_data_source.py:69:
self._static_resources = '%sstatic' % base_path
On 2013/05/03 19:12:22, kalman wrote:
> Pass static resources through rather than base_path?

Done.

[not at google - send to devlin, 2013-05-04 06:47:43]

A couple more comments. I patched this in (side note: there were warnings about
blank lines) and uploaded to the prod server to try it out.

(1) I tried putting in a bogus issue number and it timed out:

http://2-0-25.chrome-apps-doc.appspot.com/_patch/2314/extensions/storage.html

  File
"/base/data/home/apps/s~chrome-apps-doc/2-0-25.367125483683920319/file_system.py",
line 59, in ReadSingle
    return self.Read([path], binary=binary).Get()[path]
  File
"/base/data/home/apps/s~chrome-apps-doc/2-0-25.367125483683920319/patched_file_system.py",
line 46, in Read
    for files in self._patcher.GetPatchedFiles():
  File
"/base/data/home/apps/s~chrome-apps-doc/2-0-25.367125483683920319/rietveld_patcher.py",
line 192, in GetPatchedFiles
    'api/%s/%s' % (self._issue, patchset)).content)
  File
"/base/data/home/apps/s~chrome-apps-doc/2-0-25.367125483683920319/appengine_url_fetcher.py",
line 36, in Fetch
    return urlfetch.fetch('%s/%s' % (self._base_path, url), headers=headers)
  File
"/python27_runtime/python27_lib/versions/1/google/appengine/api/urlfetch.py",
line 270, in fetch
    return rpc.get_result()

(2) Then I tried with a real change and it took about 15 seconds to load,
subsequent reloads were instant:

http://2-0-25.chrome-apps-doc.appspot.com/_patch/14722012/extensions/storage....

(3) I then uploaded a new version of that patch and it didn't update straight
away.

It was awesome playing with this. It'd be nice to look into why (2) took so
long, maybe codereview is just really slow - dang - or maybe we're doing
something inefficient. If the former, it may be interesting balancing this with
(3) as we've been discussing.

(1) was unexpected, we need to fix that.

I will do some more playing tomorrow maybe - I'd like to play with adding and
deleting files.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patched_file_system_test.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/patched_file_system_test.py:22: return '1'
On 2013/05/04 02:05:41, 方觉(Fang Jue) wrote:
> On 2013/05/03 19:12:22, kalman wrote:
> > ditto not used
> 
> But GetVersion must be implemented.

Getting ahead of myself.

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
File chrome/common/extensions/docs/server2/rietveld_patcher.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/74001/chrome/common/exte...
chrome/common/extensions/docs/server2/rietveld_patcher.py:170: if datetime.now()
- time < _ISSUE_CACHE_MAXAGE:
On 2013/05/04 02:05:41, 方觉(Fang Jue) wrote:
> > Another thing that would let you do - clear that cache on every request that
> > goes into PatchServlet. I don't think that would greatly hurt performance,
and
> > it would be more useful of a feature if the patch updated immediately.
> 
> Maybe GetPatchset without cache when constructing RietveldPatcher? List of
files
> in a patchset and their content cannot change after a patchset is uploaded.

but new patches can be uploaded. That part needs to be as quick as possible.
(not sure I understand what you're saying).

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
chrome/common/extensions/docs/server2/app.yaml:2: version: 2-0-23
ok, time to bump the version... to 2-0-25 because I forgot to bump it to 2-0-24
earlier myself.

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patcher.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
chrome/common/extensions/docs/server2/patcher.py:1: class Patcher(object):
need copyright and class comment

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
chrome/common/extensions/docs/server2/patcher.py:9: pathcer.
patcher

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
chrome/common/extensions/docs/server2/patcher.py:18: 
trailing \n

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
chrome/common/extensions/docs/server2/server_instance.py:60: svn_file_system =
CachingFileSystem(OfflineFileSystem(SubversionFileSystem),
moving this up here seems cosmetic - please move it back to minimise my merge
conflicts.

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
chrome/common/extensions/docs/server2/template_data_source.py:55:
static_path=None):
just force callers to pass it in non-optionally. it's only 1 more place to
update (the test).

[方觉(Fang Jue), 2013-05-04 07:26:57]

> (2) Then I tried with a real change and it took about 15 seconds to load,
> subsequent reloads were instant:
> 
>
http://2-0-25.chrome-apps-doc.appspot.com/_patch/14722012/extensions/storage....
> 

I tried it on my own server and it also took about 15 seconds too. Logs:

2013-05-04 15:10:27.703 /_patch/14722012/extensions/storage.html 200 13319ms 8kb
Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.31 (KHTML, like Gecko)
Chrome/26.0.1410.63 Safari/537.31
2013-05-04 15:10:14.843 Fetch
https://chromiumcodereview.appspot.com/api/14722012
I 2013-05-04 15:10:14.961 Fetch
https://chromiumcodereview.appspot.com/api/14722012/2002
I 2013-05-04 15:10:16.209 Fetching tarball/14722012/2002
I 2013-05-04 15:10:22.004 Fetching samples of extensions
I 2013-05-04 15:10:22.005 Fetching https://...
I 2013-05-04 15:10:22.114 Result code of the fetch: 200
I 2013-05-04 15:10:23.499 Serving samples of extensions from memcache

(I made SamplesDataSource.get fetching pre-generated json from another server.)

I'll add more logging.info and gather more data.

> (1) I tried putting in a bogus issue number and it timed out:

Also tried it on my own server. It didn't time out though. From the log,
there're a lot of requests to
https://chromiumcodereview.appspot.com/api/2314
and even https://chromiumcodereview.appspot.com/api/2314/None
There's something that need fixing.

> (3) I then uploaded a new version of that patch and it didn't update straight
away.
It should update in no more than five minutes. I tested this case yesterday.

> but new patches can be uploaded. That part needs to be as quick as possible.
(not sure I understand what you're saying).
I mean
class RietveldPatcher(Patcher):
  def __init__(self, ...):
    self._version = self._GetPatchsetWithoutCache()
  def GetVersion():
    return self._version
RietveldPatcher will be constructed for every request in PatchServlet so if any
update comes in, the version changes and new data will be fetched.

[方觉(Fang Jue), 2013-05-04 08:20:02]

Some data:

2013-05-04 16:05:51.180 /_patch/14672007/extensions/faq.html 200 8443ms 9kb
Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.31 (KHTML, like Gecko)
Chrome/26.0.1410.63 Safari/537.31
I 2013-05-04 16:05:42.789 Fetch
https://chromiumcodereview.appspot.com/api/14672007
I 2013-05-04 16:05:42.928 Fetch
https://chromiumcodereview.appspot.com/api/14672007/1
I 2013-05-04 16:05:43.390 Fetching tarball/14672007/1
I 2013-05-04 16:05:43.391 GetMissingPaths begin
I 2013-05-04 16:05:43.556 talball fetched
I 2013-05-04 16:05:43.573 GetMissingPaths end

2013-05-04 16:07:00.293 /_patch/14672007a/extensions/faq.html 200 8072ms 9kb
Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.31 (KHTML, like Gecko)
Chrome/26.0.1410.63 Safari/537.31
I 2013-05-04 16:06:52.267 Fetch
https://chromiumcodereview.appspot.com/api/14672007a

.Rendering apps/faq.html...
Took 4.34627699852 seconds
Rendering extensions/faq.html...
Took 0.463413000107 seconds
(render locally in the presubmit of the patch)

I think most of time is spent on rendering. Codereview and extracting tarball
doesn't seem to be slow. Everything in CompiledFileSystem needs to be rebuilt
for every issue.

[方觉(Fang Jue), 2013-05-04 10:47:17]

I did a quick hack on CompiledFileSystem and reduced the latency to ~5 sec.
Meanwhile, cache timeout for issue's patchset number becomes 5 seconds.

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
File chrome/common/extensions/docs/server2/patcher.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
chrome/common/extensions/docs/server2/patcher.py:1: class Patcher(object):
On 2013/05/04 06:47:43, kalman wrote:
> need copyright and class comment

oops. Done.

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
chrome/common/extensions/docs/server2/patcher.py:9: pathcer.
On 2013/05/04 06:47:43, kalman wrote:
> patcher

ah..

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
chrome/common/extensions/docs/server2/patcher.py:18: 
On 2013/05/04 06:47:43, kalman wrote:
> trailing \n

Done.

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
chrome/common/extensions/docs/server2/server_instance.py:60: svn_file_system =
CachingFileSystem(OfflineFileSystem(SubversionFileSystem),
On 2013/05/04 06:47:43, kalman wrote:
> moving this up here seems cosmetic - please move it back to minimise my merge
> conflicts.

It was moved because when I did this there wasn't _CreateWithPatch. It's moved
back now.

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/83001/chrome/common/exte...
chrome/common/extensions/docs/server2/template_data_source.py:55:
static_path=None):
On 2013/05/04 06:47:43, kalman wrote:
> just force callers to pass it in non-optionally. it's only 1 more place to
> update (the test).

Done.

[方觉(Fang Jue), 2013-05-05 14:04:36]

Updates:
1. I solved the inefficiency by making CompiledFileSystem chained to the
unpatched CompiledFileSystem (see changes in compiled_file_system.py). The idea
behind this is still to reuse existing cached data (as I did at the very
beginning, though the method is completely different).

2. RietveldPatcher now throws an exception handled by PatchServlet to show error
messages when something wrong happens (including invalid issue number).

3. Cache timeout for issue's patchset number becomes 5 seconds. You'll see
updates almost instantly after uploading a new patchset.

4. rietveld_patcher.py is refactored to separate cache logic .

5. patched_file_system_test is improved and covers many cases now. It doesn't
rely on SubversionFileSystem any more.  Also fixed a couple of bugs in
patched_file_system discovered during testing.

6. I also did much playing today. I wrote a small user script
(https://docs.google.com/file/d/0B75njwESxhdlLUdhMG93OVloMkk/edit?usp=sharing)
that adds links to codereview page to make it more convenient to open patched
docs.

[not at google - send to devlin, 2013-05-07 05:45:40]

I'm trying to finish this restructure asap to make your life easier, just,
writing test now.

https://codereview.chromium.org/14125010/diff/123001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/compiled_file_system.py (right):

https://codereview.chromium.org/14125010/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/server2/compiled_file_system.py:22:
patched_object_store_creator_factory=None):
this class is conceptually quite simple and now... it isn't. such a generic
class shouldn't have knowledge of server implementation details such as there
being such a thing as a "patched file system".

is there a different way you can achieve what you're going for? which is...
using the old data for the stuff which hasn't been patched, right?

like I said... is it really such a big deal? how much difference does it make?
can you measure it beyond just the first load (which I wouldn't expect to be so
bad since you can use the existing data anyway)? is it just a matter of saving
datastore space, because we really do have infinity storage basically.

https://codereview.chromium.org/14125010/diff/123001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/patch_servlet.py (right):

https://codereview.chromium.org/14125010/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/server2/patch_servlet.py:17: if '/' in
path_with_issue:
well if there isn't a '/' then it's a malformed, so we can return NotFound right
away.

https://codereview.chromium.org/14125010/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/server2/patch_servlet.py:33: # Disable cache for
patched content.
you can do "response.headers.pop('cache-control', None)"

https://codereview.chromium.org/14125010/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/server2/patch_servlet.py:37: response =
Response.NotFound(e.message, {'Content-Type': 'text/plain'})
would kind of rather prefer to leave the content-type out of here, it's
hopefully unnecessary.

https://codereview.chromium.org/14125010/diff/123001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/patched_file_system_test.py (right):

https://codereview.chromium.org/14125010/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/server2/patched_file_system_test.py:217: 
sweet tests

https://codereview.chromium.org/14125010/diff/123001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/rietveld_patcher.py (right):

https://codereview.chromium.org/14125010/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/server2/rietveld_patcher.py:137: class
_Cache(object):
the caching logic should all be pulled out into a decorator, in the same way
that caching for object stores and file systems are done. that will let you test
it separately (albeit the test for caching should be pretty simple) - it won't
get in the way of testing the rietveld patcher.

class CachingPatcher(Patcher):
  def __init__(self, patcher, object_store_creator_factory):
    self._patcher = patcher
    self._object_store_creator_factory = oscf

  def GetPatchedFiles(self):
    ... consult cache
    ... get from patcher

  ... same

then in ServerInstance when you construct a RietveldPatcher, instead construct a
CachingPatcher(RietveldPatcher(),oscf)

https://codereview.chromium.org/14125010/diff/123001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://codereview.chromium.org/14125010/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/server2/server_instance.py:51: def
GetOrCreateOffline(channel, static_path=None, issue=None):
these optional parameters won't work well with the memoization, but I've...
almost... finished simplifying the way this stuff works and that'll let you do
this much more easily.

[方觉(Fang Jue), 2013-05-07 06:03:55]

https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
File chrome/common/extensions/docs/server2/compiled_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
chrome/common/extensions/docs/server2/compiled_file_system.py:22:
patched_object_store_creator_factory=None):
On 2013/05/07 05:45:40, kalman wrote:
> this class is conceptually quite simple and now... it isn't. such a generic
> class shouldn't have knowledge of server implementation details such as there
> being such a thing as a "patched file system".
> 
> is there a different way you can achieve what you're going for? which is...
> using the old data for the stuff which hasn't been patched, right?
> 
> like I said... is it really such a big deal? how much difference does it make?
> can you measure it beyond just the first load (which I wouldn't expect to be
so
> bad since you can use the existing data anyway)? is it just a matter of saving
> datastore space, because we really do have infinity storage basically.

It's definitely not a good way to achieve it. What I think would be better is to
add a class called ChainedCompiledFileSystem (or something like that) and handle
these things there. I'm still thinking about how to do that.

For your 'storagetest' patch, without this change the first load takes about 15
seconds. With this change, it usually takes less than 5 seconds. So it's not a
small deal.

https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
File chrome/common/extensions/docs/server2/patch_servlet.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
chrome/common/extensions/docs/server2/patch_servlet.py:17: if '/' in
path_with_issue:
On 2013/05/07 05:45:40, kalman wrote:
> well if there isn't a '/' then it's a malformed, so we can return NotFound
right
> away.

Done.

https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
chrome/common/extensions/docs/server2/patch_servlet.py:33: # Disable cache for
patched content.
On 2013/05/07 05:45:40, kalman wrote:
> you can do "response.headers.pop('cache-control', None)"

Done.

https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
chrome/common/extensions/docs/server2/patch_servlet.py:37: response =
Response.NotFound(e.message, {'Content-Type': 'text/plain'})
On 2013/05/07 05:45:40, kalman wrote:
> would kind of rather prefer to leave the content-type out of here, it's
> hopefully unnecessary.

I just don't want any HTML injected in URL to be returned here and intepreted in
the browser.

https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
File chrome/common/extensions/docs/server2/rietveld_patcher.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
chrome/common/extensions/docs/server2/rietveld_patcher.py:137: class
_Cache(object):
On 2013/05/07 05:45:40, kalman wrote:
> the caching logic should all be pulled out into a decorator, in the same way
> that caching for object stores and file systems are done. that will let you
test
> it separately (albeit the test for caching should be pretty simple) - it won't
> get in the way of testing the rietveld patcher.
> 
> class CachingPatcher(Patcher):
>   def __init__(self, patcher, object_store_creator_factory):
>     self._patcher = patcher
>     self._object_store_creator_factory = oscf
> 
>   def GetPatchedFiles(self):
>     ... consult cache
>     ... get from patcher
> 
>   ... same
> 
> then in ServerInstance when you construct a RietveldPatcher, instead construct
a
> CachingPatcher(RietveldPatcher(),oscf)

I'll try to do that. But I don't think it's possible to write a generic
CachingPatcher. RietveldPatcher fetches all patched files at a time but this may
not be true for some other patchers.

[not at google - send to devlin, 2013-05-07 06:14:31]

https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
File chrome/common/extensions/docs/server2/compiled_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
chrome/common/extensions/docs/server2/compiled_file_system.py:22:
patched_object_store_creator_factory=None):
> It's definitely not a good way to achieve it. What I think would be better is
to
> add a class called ChainedCompiledFileSystem (or something like that) and
handle
> these things there. I'm still thinking about how to do that.

That sounds interesting, though it seems to me like the problem is at the
FileSystem not the CompiledFileSystem level, i.e. what sounds more promising is
a CacheChainFilleSystem or something (using the same analogy). I wouldn't want
to modify CachingFileSystem at all - it's already complex enough - so maybe just
a ChainedFileSystem would be enough, which tries one after another (it would be
constructed with the patched then the compiled obviously). Make
PatchedFileSystem throw FileNotFound errors for files it doesn't have
modifications for (hm, though you'd need to distinguish between that and the
file being removed).

Anyway, I'll leave it to you. Getting late here, brain not 100%.

> 
> For your 'storagetest' patch, without this change the first load takes about
15
> seconds. With this change, it usually takes less than 5 seconds. So it's not a
> small deal.

Ok. Are subsequent loads the same? I presume so. What about loads after there
has been a patch update? That is an important question. I would expect them to
be quite comparable to each other, very quick.

[方觉(Fang Jue), 2013-05-07 06:14:46]

Oh, there's another point to mention. If a patch adds docs for some new API
without modifying its json/idl file, it doesn't work because the json/idl (that
was undocumented) was not pulled into data store in cron.

[not at google - send to devlin, 2013-05-07 06:17:21]

https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
File chrome/common/extensions/docs/server2/patch_servlet.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
chrome/common/extensions/docs/server2/patch_servlet.py:37: response =
Response.NotFound(e.message, {'Content-Type': 'text/plain'})
On 2013/05/07 06:03:55, 方觉(Fang Jue) wrote:
> On 2013/05/07 05:45:40, kalman wrote:
> > would kind of rather prefer to leave the content-type out of here, it's
> > hopefully unnecessary.
> 
> I just don't want any HTML injected in URL to be returned here and intepreted
in
> the browser.

Ok cool. We should either enforce that there's a content type specified in the
response (probably via separating it out into a separate parameter) or make it
default to text/plain, or both.

https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
File chrome/common/extensions/docs/server2/rietveld_patcher.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
chrome/common/extensions/docs/server2/rietveld_patcher.py:137: class
_Cache(object):
On 2013/05/07 06:03:55, 方觉(Fang Jue) wrote:
> On 2013/05/07 05:45:40, kalman wrote:
> > the caching logic should all be pulled out into a decorator, in the same way
> > that caching for object stores and file systems are done. that will let you
> test
> > it separately (albeit the test for caching should be pretty simple) - it
won't
> > get in the way of testing the rietveld patcher.
> > 
> > class CachingPatcher(Patcher):
> >   def __init__(self, patcher, object_store_creator_factory):
> >     self._patcher = patcher
> >     self._object_store_creator_factory = oscf
> > 
> >   def GetPatchedFiles(self):
> >     ... consult cache
> >     ... get from patcher
> > 
> >   ... same
> > 
> > then in ServerInstance when you construct a RietveldPatcher, instead
construct
> a
> > CachingPatcher(RietveldPatcher(),oscf)
> 
> I'll try to do that. But I don't think it's possible to write a generic
> CachingPatcher. RietveldPatcher fetches all patched files at a time but this
may
> not be true for some other patchers.

What do you mean "fetches all patched files at a time"? Why does that make
caching harder?

[方觉(Fang Jue), 2013-05-07 06:19:28]

On 2013/05/07 06:14:31, kalman wrote:
>
https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
> File chrome/common/extensions/docs/server2/compiled_file_system.py (right):
> 
>
https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
> chrome/common/extensions/docs/server2/compiled_file_system.py:22:
> patched_object_store_creator_factory=None):
> > It's definitely not a good way to achieve it. What I think would be better
is
> to
> > add a class called ChainedCompiledFileSystem (or something like that) and
> handle
> > these things there. I'm still thinking about how to do that.
> 
> That sounds interesting, though it seems to me like the problem is at the
> FileSystem not the CompiledFileSystem level, i.e. what sounds more promising
is
> a CacheChainFilleSystem or something (using the same analogy). I wouldn't want
> to modify CachingFileSystem at all - it's already complex enough - so maybe
just
> a ChainedFileSystem would be enough, which tries one after another (it would
be
> constructed with the patched then the compiled obviously). Make
> PatchedFileSystem throw FileNotFound errors for files it doesn't have
> modifications for (hm, though you'd need to distinguish between that and the
> file being removed).
> 
> Anyway, I'll leave it to you. Getting late here, brain not 100%.

No. It should chain two compiled file systems (CompiledFileSystem is not
inherited from FileSystem). Changes to CompiledFileSystem are still necessary to
expose cache entries' versions and file system's stat.
PatchedFileSystem is actually already a 'chained' file system (it chains svn
file system with rietveld patcher!).

Good night!

> 
> > 
> > For your 'storagetest' patch, without this change the first load takes about
> 15
> > seconds. With this change, it usually takes less than 5 seconds. So it's not
a
> > small deal.
> 
> Ok. Are subsequent loads the same? I presume so. What about loads after there
> has been a patch update? That is an important question. I would expect them to
> be quite comparable to each other, very quick.

Subsequent loads are not slow as you tested because those data in compiled file
system are already cached.

Uploading a new patchset is effectively the same as creating a new issue.

[方觉(Fang Jue), 2013-05-07 06:21:52]

https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
> File chrome/common/extensions/docs/server2/rietveld_patcher.py (right):
> 
>
https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
> chrome/common/extensions/docs/server2/rietveld_patcher.py:137: class
> _Cache(object):
> On 2013/05/07 06:03:55, 方觉(Fang Jue) wrote:
> > On 2013/05/07 05:45:40, kalman wrote:
> > > the caching logic should all be pulled out into a decorator, in the same
way
> > > that caching for object stores and file systems are done. that will let
you
> > test
> > > it separately (albeit the test for caching should be pretty simple) - it
> won't
> > > get in the way of testing the rietveld patcher.
> > > 
> > > class CachingPatcher(Patcher):
> > >   def __init__(self, patcher, object_store_creator_factory):
> > >     self._patcher = patcher
> > >     self._object_store_creator_factory = oscf
> > > 
> > >   def GetPatchedFiles(self):
> > >     ... consult cache
> > >     ... get from patcher
> > > 
> > >   ... same
> > > 
> > > then in ServerInstance when you construct a RietveldPatcher, instead
> construct
> > a
> > > CachingPatcher(RietveldPatcher(),oscf)
> > 
> > I'll try to do that. But I don't think it's possible to write a generic
> > CachingPatcher. RietveldPatcher fetches all patched files at a time but this
> may
> > not be true for some other patchers.
> 
> What do you mean "fetches all patched files at a time"? Why does that make
> caching harder?

Because we fetch the tarball and then extract files. It is effectively the same
as fetching all files. It means that the CachingRietveldPatcher should ask for
all files from RietveldPatcher if any file to read is missing from cache (and
the next time all files are cached and directly read from cache). That's the
main difference from CachingFileSystem.

[not at google - send to devlin, 2013-05-07 06:24:39]

> 
> Because we fetch the tarball and then extract files. It is effectively the
same
> as fetching all files. It means that the CachingRietveldPatcher should ask for
> all files from RietveldPatcher if any file to read is missing from cache (and
> the next time all files are cached and directly read from cache). That's the
> main difference from CachingFileSystem.

I see. We have a similar situation in GithubFileSystem where all the content is
fetched in a zip file then dispensed separately. The RietveldFileSystem could do
something similar - lazily fetch all the files, then leave the per-file
computation caching to the Caching version.

GithubFileSystem isn't quite implemented that way yet, there are at least 3
TODOs around the place for me to fix that.

[not at google - send to devlin, 2013-05-07 06:32:10]

On 2013/05/07 06:19:28, 方觉(Fang Jue) wrote:
> On 2013/05/07 06:14:31, kalman wrote:
> >
>
https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
> > File chrome/common/extensions/docs/server2/compiled_file_system.py (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/14125010/diff/123001/chrome/common/ext...
> > chrome/common/extensions/docs/server2/compiled_file_system.py:22:
> > patched_object_store_creator_factory=None):
> > > It's definitely not a good way to achieve it. What I think would be better
> is
> > to
> > > add a class called ChainedCompiledFileSystem (or something like that) and
> > handle
> > > these things there. I'm still thinking about how to do that.
> > 
> > That sounds interesting, though it seems to me like the problem is at the
> > FileSystem not the CompiledFileSystem level, i.e. what sounds more promising
> is
> > a CacheChainFilleSystem or something (using the same analogy). I wouldn't
want
> > to modify CachingFileSystem at all - it's already complex enough - so maybe
> just
> > a ChainedFileSystem would be enough, which tries one after another (it would
> be
> > constructed with the patched then the compiled obviously). Make
> > PatchedFileSystem throw FileNotFound errors for files it doesn't have
> > modifications for (hm, though you'd need to distinguish between that and the
> > file being removed).
> > 
> > Anyway, I'll leave it to you. Getting late here, brain not 100%.
> 
> No. It should chain two compiled file systems (CompiledFileSystem is not
> inherited from FileSystem). Changes to CompiledFileSystem are still necessary
to
> expose cache entries' versions and file system's stat.
> PatchedFileSystem is actually already a 'chained' file system (it chains svn
> file system with rietveld patcher!).

Heh, true, sorry. And yep I'm well aware of the differences between CompiledFS
and FS.

Anyway - so this would imply to me that there's an extra ~10s involved in
compiling these files? That's quite amazing. Maybe that's something to look at
profiling and fixing.

> 
> Good night!
> 

Thanks :) logging of for real now.

> Subsequent loads are not slow as you tested because those data in compiled
file
> system are already cached.
> 
> Uploading a new patchset is effectively the same as creating a new issue.

CachingFileSystem tries to be intelligent and only re-requests the files that
have changed, so I'd expect subsequent loads to be fast, since the
PatchedFileSystem only needs to bump the Stat value.

[方觉(Fang Jue), 2013-05-07 14:28:59]

CachingRietveldPatcher is separated from RietveldPatcher.
ChainedCompiledFileSystem is added. Tests for them are also written.

> I see. We have a similar situation in GithubFileSystem where all the content
is
> fetched in a zip file then dispensed separately. The RietveldFileSystem could
do
> something similar - lazily fetch all the files, then leave the per-file
> computation caching to the Caching version.
>
> GithubFileSystem isn't quite implemented that way yet, there are at least 3
> TODOs around the place for me to fix that.

It's somewhat similar but not the same. GithubFileSystem's zipball is very large
and cached (but individual files are not) while RietveldPatcher's tarball is
very small and not cached (while individual files are).

> Oh, there's another point to mention. If a patch adds docs for some new API
> without modifying its json/idl file, it doesn't work because the json/idl
(that
> was undocumented) was not pulled into data store in cron.

Repeat in case you missed it.

[not at google - send to devlin, 2013-05-07 14:57:31]

> 
> It's somewhat similar but not the same. GithubFileSystem's zipball is very
large
> and cached (but individual files are not) while RietveldPatcher's tarball is
> very small and not cached (while individual files are).

Yep - like I said there are TODOs in GithubFileSystem to model it this way -
check zipball into blobstore and cache individual files - it's a bug that it
doesn't do that at the moment.

RietveldPatcher might as well be the same - it doesn't require using blobstore,
just datastore, but the principal is the same; there is comparatively last
one-time cost because of the fetch.

> 
> > Oh, there's another point to mention. If a patch adds docs for some new API
> > without modifying its json/idl file, it doesn't work because the json/idl
> (that
> > was undocumented) was not pulled into data store in cron.
> 
> Repeat in case you missed it.

I missed it, sorry. I see. So I add docs for a new API, like

{{+standard_extensions_api api:foo}}

but the schema for foo hasn't been seen by the cron, so it doesn't work? Hm.
That kills one of the main use cases for this feature, reviewing docs for new
APIs. I see two solutions to this, (1) *not* use an OfflineFileSystem for the
patch (this should be ok right?) or (2) make the crons pull in all of the
schema.

(1) should be easy enough for now? However, (2) is actually the 2nd or 3rd time
I've wanted this - random classes wanting to cache things in cron - I was
thinking about how to make this more componentisable, but I won't get around to
it for a few days at least.

[方觉(Fang Jue), 2013-05-07 23:25:36]

> > 
> > > Oh, there's another point to mention. If a patch adds docs for some new
API
> > > without modifying its json/idl file, it doesn't work because the json/idl
> > (that
> > > was undocumented) was not pulled into data store in cron.
> > 
> > Repeat in case you missed it.
> 
> I missed it, sorry. I see. So I add docs for a new API, like
> 
> {{+standard_extensions_api api:foo}}
> 
> but the schema for foo hasn't been seen by the cron, so it doesn't work? Hm.
> That kills one of the main use cases for this feature, reviewing docs for new
> APIs. I see two solutions to this, (1) *not* use an OfflineFileSystem for the
> patch (this should be ok right?) or (2) make the crons pull in all of the
> schema.
> 
> (1) should be easy enough for now? However, (2) is actually the 2nd or 3rd
time
> I've wanted this - random classes wanting to cache things in cron - I was
> thinking about how to make this more componentisable, but I won't get around
to
> it for a few days at least.

Some data with online file system (11478051 is a 'new' patch that has never been
fetched by the test server):
2013-05-08 07:11:08.778 /_patch/11478051/apps/about_apps.html 200 11476ms 4kb
Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.31 (KHTML, like Gecko)
Chrome/26.0.1410.63 Safari/537.31
(20+ stat requests to svn)
(static resources follow, each had one stat request to svn. Time varied from
260ms to 3710 ms.)
2013-05-08 07:12:44.012 /_patch/11478051/apps/app_deprecated.html 200 4684ms 4kb
Mozilla/5.0 (X11; Linux i686) AppleWebKit
(static resources...)
Subsequent requests to html took about 4 seconds.

[not at google - send to devlin, 2013-05-08 15:02:17]

> Some data with online file system (11478051 is a 'new' patch that has never
been
> fetched by the test server):
> 2013-05-08 07:11:08.778 /_patch/11478051/apps/about_apps.html 200 11476ms 4kb
> Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.31 (KHTML, like Gecko)
> Chrome/26.0.1410.63 Safari/537.31
> (20+ stat requests to svn)
> (static resources follow, each had one stat request to svn. Time varied from
> 260ms to 3710 ms.)
> 2013-05-08 07:12:44.012 /_patch/11478051/apps/app_deprecated.html 200 4684ms
4kb
> Mozilla/5.0 (X11; Linux i686) AppleWebKit
> (static resources...)
> Subsequent requests to html took about 4 seconds.

You've convinced me that building support for this stuff into CompiledFileSystem
is a good idea, but as I think you agree, it should be done in a more generic
way (e.g. by specifying a chain of file systems, a generalisation of what you
have really).

And thanks for looking into those stats, though I don't know how to interpret
them. 10s to fetch? What is it when it's offline? Either way it's looking like
(2) is the way to go, I'll bump that up in priority (will do after the
Subversion pinning stuff), but can we submit with this bug (assuming it won't
fundamentally change the architecture) and then working on fixing it next?

[方觉(Fang Jue), 2013-05-08 23:09:34]

On 2013/05/08 15:02:17, kalman wrote:
> > Some data with online file system (11478051 is a 'new' patch that has never
> been
> > fetched by the test server):
> > 2013-05-08 07:11:08.778 /_patch/11478051/apps/about_apps.html 200 11476ms
4kb
> > Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.31 (KHTML, like Gecko)
> > Chrome/26.0.1410.63 Safari/537.31
> > (20+ stat requests to svn)
> > (static resources follow, each had one stat request to svn. Time varied from
> > 260ms to 3710 ms.)
> > 2013-05-08 07:12:44.012 /_patch/11478051/apps/app_deprecated.html 200 4684ms
> 4kb
> > Mozilla/5.0 (X11; Linux i686) AppleWebKit
> > (static resources...)
> > Subsequent requests to html took about 4 seconds.
> 
> You've convinced me that building support for this stuff into
CompiledFileSystem
> is a good idea, but as I think you agree, it should be done in a more generic
> way (e.g. by specifying a chain of file systems, a generalisation of what you
> have really).
> 
> And thanks for looking into those stats, though I don't know how to interpret
> them. 10s to fetch? What is it when it's offline? Either way it's looking like
> (2) is the way to go, I'll bump that up in priority (will do after the
> Subversion pinning stuff), but can we submit with this bug (assuming it won't
> fundamentally change the architecture) and then working on fixing it next?

When offline, stats are directly read from cache. When online, it's fetched from
svn. Every request requires at least one stat.
After I rebase, it should be ready to submit (if things added in patchset 34 and
later are ok). And should we use online file system or offline for now?

[not at google - send to devlin, 2013-05-08 23:12:10]

On 2013/05/08 23:09:34, 方觉(Fang Jue) wrote:
> On 2013/05/08 15:02:17, kalman wrote:
> > > Some data with online file system (11478051 is a 'new' patch that has
never
> > been
> > > fetched by the test server):
> > > 2013-05-08 07:11:08.778 /_patch/11478051/apps/about_apps.html 200 11476ms
> 4kb
> > > Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.31 (KHTML, like Gecko)
> > > Chrome/26.0.1410.63 Safari/537.31
> > > (20+ stat requests to svn)
> > > (static resources follow, each had one stat request to svn. Time varied
from
> > > 260ms to 3710 ms.)
> > > 2013-05-08 07:12:44.012 /_patch/11478051/apps/app_deprecated.html 200
4684ms
> > 4kb
> > > Mozilla/5.0 (X11; Linux i686) AppleWebKit
> > > (static resources...)
> > > Subsequent requests to html took about 4 seconds.
> > 
> > You've convinced me that building support for this stuff into
> CompiledFileSystem
> > is a good idea, but as I think you agree, it should be done in a more
generic
> > way (e.g. by specifying a chain of file systems, a generalisation of what
you
> > have really).
> > 
> > And thanks for looking into those stats, though I don't know how to
interpret
> > them. 10s to fetch? What is it when it's offline? Either way it's looking
like
> > (2) is the way to go, I'll bump that up in priority (will do after the
> > Subversion pinning stuff), but can we submit with this bug (assuming it
won't
> > fundamentally change the architecture) and then working on fixing it next?
> 
> When offline, stats are directly read from cache. When online, it's fetched
from
> svn. Every request requires at least one stat.
> After I rebase, it should be ready to submit (if things added in patchset 34
and
> later are ok). And should we use online file system or offline for now?

Yep. Let's just use online then fix the speed issue afterwards. Having this
stuff work for all cases is better than a 10s slowdown for now.

[方觉(Fang Jue), 2013-05-09 14:50:13]

On 2013/05/08 23:12:10, kalman wrote:
> On 2013/05/08 23:09:34, 方觉(Fang Jue) wrote:
> > On 2013/05/08 15:02:17, kalman wrote:
> > > > Some data with online file system (11478051 is a 'new' patch that has
> never
> > > been
> > > > fetched by the test server):
> > > > 2013-05-08 07:11:08.778 /_patch/11478051/apps/about_apps.html 200
11476ms
> > 4kb
> > > > Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.31 (KHTML, like Gecko)
> > > > Chrome/26.0.1410.63 Safari/537.31
> > > > (20+ stat requests to svn)
> > > > (static resources follow, each had one stat request to svn. Time varied
> from
> > > > 260ms to 3710 ms.)
> > > > 2013-05-08 07:12:44.012 /_patch/11478051/apps/app_deprecated.html 200
> 4684ms
> > > 4kb
> > > > Mozilla/5.0 (X11; Linux i686) AppleWebKit
> > > > (static resources...)
> > > > Subsequent requests to html took about 4 seconds.
> > > 
> > > You've convinced me that building support for this stuff into
> > CompiledFileSystem
> > > is a good idea, but as I think you agree, it should be done in a more
> generic
> > > way (e.g. by specifying a chain of file systems, a generalisation of what
> you
> > > have really).
> > > 
> > > And thanks for looking into those stats, though I don't know how to
> interpret
> > > them. 10s to fetch? What is it when it's offline? Either way it's looking
> like
> > > (2) is the way to go, I'll bump that up in priority (will do after the
> > > Subversion pinning stuff), but can we submit with this bug (assuming it
> won't
> > > fundamentally change the architecture) and then working on fixing it next?
> > 
> > When offline, stats are directly read from cache. When online, it's fetched
> from
> > svn. Every request requires at least one stat.
> > After I rebase, it should be ready to submit (if things added in patchset 34
> and
> > later are ok). And should we use online file system or offline for now?
> 
> Yep. Let's just use online then fix the speed issue afterwards. Having this
> stuff work for all cases is better than a 10s slowdown for now.

After the refactory, ObjectStoreCreator is created in patch_servlet and it can
be created with start_empty=False. Then a CachingFileSystem created with that
object store creator is exactly what I want: read & stat from cache, request svn
for files not cached (though I haven't yet tested). The problem of offline file
system should go away.

Give me a couple of minutes to test and upload new patchset.

[not at google - send to devlin, 2013-05-11 20:39:53]

I think this will be the last round. Sorry for taking so long to review this.

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
File chrome/common/extensions/docs/server2/caching_rietveld_patcher.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:16: if
isinstance(path_or_paths, list) or isinstance(path_or_paths, set):
isinstance(path_or_paths, (tuple, list, set))

that said, it's nicer for functions to accept a single type and either
- this method can only take a sequence, and _ToObjectStoreValue does
_MakeKey([key])[0] - in which case call this MakeKeys.
- this takes a single value and where it's used below does a list comprehension

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:24: return
{key[0:key.find('@')]: _HandleBinary(raw_value[key], binary)
What happens if a patchset contains both binary and non-binary data? Do we need
to handle that at all? Can it happen?

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:25: for key in
raw_value}
btw @ is a legitimate character to appear in a file path, should be rfind('@').

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:63: class
assumes that applying to all patched files one at a time is more
s/one at a time/at once/

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:79: value =
self._list_object_store.Get(key).Get()
Re-using list_object_store here and in GetPatchedFiles is a bit confusing.
Having an object store where you only ever access a single key is fine,
version_object_store?

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:90: def
GetPatchedFiles(self, version = None):
version=None

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:97: value =
self._patcher.GetPatchedFiles(version)
more descriptive name than "value" pls

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:119: version),
why is binary explicitly True?

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
File chrome/common/extensions/docs/server2/caching_rietveld_patcher_test.py
(right):

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/caching_rietveld_patcher_test.py:15: class
TestPatcher(Patcher):
pull this into its own file. maybe a class in the future will want one.

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
File chrome/common/extensions/docs/server2/chained_compiled_file_system.py
(right):

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:8: class
ChainedCompiledFileSystem(CompiledFileSystem):
comment

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:8: class
ChainedCompiledFileSystem(CompiledFileSystem):
pls always use composition not inheritance - particularly in python, in which
object graphs are confusing enough already.

the only exception to this is interfaces/mixins like
FileSystem/ObjectStore/Patcher/etc are. You will notice there is no case of
implementation inheritance in the docserver, except where enforced by libraries.

What I imagine this class might look like:

class ChainedCompiledFileSystem(object):
  # Comment like...
  '''A CompiledFileSystem implementation which gets its data from a chain of
delegate
  CompiledFileSystems. ... the reason why you need it...
  '''
  def Factory(object):
    def __init__(self, compiled_fs_factory_chain):
      self._compiled_fs_factory_chain = compiled_fs_factory_chain

    def Create(self, populate_function, cls, category=None):
      compiled_fs_chain = [factory(populate_function, cls, category)
                           for factory in self._compiled_fs_factory_chain]
      return ChainedCompiledFileSystem(compiled_fs_chain)

  def __init__(self, compiled_fs_chain):
    self._compiled_fs_chain = compiled_fs_chain

  def GetFromFile(self, path, binary=False):
    for compiled_fs in self._compiled_fs_chain:
      try:
        return compiled_fs.GetFromFile(path, binary)
      except FileNotFoundError:
        continue
    # For stack trace.
    self._compiled_fs_chain[-1].GetFromFile(path, binary)
    raise AssertionError()

  def GetFromFileListing(self, path):
    ...

which does away with file_system - but from what I can tell, if the chain passed
into this file has the patched file system first, it will get that version
first?

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:24: 
seems to me like this class could just as easily compose the file system that it
inherits from?

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
File chrome/common/extensions/docs/server2/chained_compiled_file_system_test.py
(right):

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/chained_compiled_file_system_test.py:68: 
test binary here as well?

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
File chrome/common/extensions/docs/server2/compiled_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/compiled_file_system.py:16: def
__init__(self, file_system, object_store_creator):
See comments in CacheChainFileSystem. I don't think this file really needs to
change.

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
File chrome/common/extensions/docs/server2/patch_servlet.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/patch_servlet.py:23: class
Delegate(InstanceServlet.Delegate):
see earlier comment about using composition over inheritance

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/patch_servlet.py:23: class
Delegate(InstanceServlet.Delegate):
Why InstanceServlet rather than RenderServlet? I was trying to make it so that
RenderServlet is the entry point for rendering things, and
Cron/Instance/PatchServlet call out to that.

Anyway, be explicit about what PatchServlet needs, and keep the implementation
of InstanceServlet's (or RenderServlet's?) Delegate separate. See how
CronServlet does it. It's not clear from looking at this what the dependency
graph looks like.

I.e. pull most of this into a separate _RenderServletDelegate class, which takes
the delegate for PatchServlet and whatever it needs (issue I suppose).

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/patch_servlet.py:28: def _GetChannel(self,
channel):
methods intended for overriding shouldn't start with a _, they're not private.

I don't understand why this method is even necessary, though. The channel is
passed in, and it's not interesting for patch servlet.

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/patch_servlet.py:62: self._delegate =
delegate
see below.

self._issue = ...
self._delegate = delegate or _RenderServletDelegate(self._issue)

etc.

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/patch_servlet.py:69: return
Response.NotFound('Malformed URL.')
Maybe print what the URL should look like

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/patch_servlet.py:74: self._delegate =
PatchServlet.Delegate(issue)
everything above here can go in the constructor.

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/patch_servlet.py:86: if
response.IsRedirect():
maybe this should actually be GetRedirect().

redirect = response.GetRedirect()
if redirect is not None:
  ...

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
File chrome/common/extensions/docs/server2/patch_servlet_test.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/patch_servlet_test.py:34: def
renderWithPatch(self, path, issue):
_RenderWithPatch

though it looks like none of these needs to be instance methods of
PatchServletTest, they can be outside.

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/patch_servlet_test.py:39: def
renderWithoutPatch(self, path):
_RenderWithoutPatch

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/patch_servlet_test.py:42: def
compare(self, patched_response, unpatched_response, issue):
_Compare, but besides "compare" usually implies a < operation not an ==. I'd
just inline it within RenderAndCompare, particularly given you have the
opportunity to be more descriptive then, namely, return a (bool, message) tuple
then when using assertTrue below, do assertTrue(*_IsRenderingEqual(...)) so that
it says what went wrong.

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/patch_servlet_test.py:51: def
renderAndCompare(self, path, issue):
_IsRenderingEqual?

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
File chrome/common/extensions/docs/server2/patched_file_system.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/patched_file_system.py:121: return
self._host_file_system.Stat(path)
I think my comments in CacheChainCompiledSystem will only work if the contract
of PatchedFileSystem is changed so that it only considers files that are within
the patch to actually exist.

So this would raise a FileNotFoundException, likewise Read, etc.

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
File chrome/common/extensions/docs/server2/patcher.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/patcher.py:7: ''' Returns (added_files,
deleted_files, modified_files).
Comment what version= means.

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/patcher.py:17: def Apply(self, paths,
file_system, binary, version=None):
I can't see file_system being used anywhere

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/patcher.py:19: data. Throws
FileNotFoundError if |paths| contains deleted files.
comment what version= means.

P.S. I was under the impression that codereview has issues with binary files?
Nevermind.

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://chromiumcodereview.appspot.com/14125010/diff/177004/chrome/common/ext...
chrome/common/extensions/docs/server2/server_instance.py:44:
compiled_fs_factory=None):
having these as optional parameters doesn't gain much, there are only a few
places where ServerInstances are constructed, and I prefer to be explicit rather
than have too many optional parameters.

[方觉(Fang Jue), 2013-05-12 03:01:47]

(new patchset uploading...)

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
File chrome/common/extensions/docs/server2/caching_rietveld_patcher.py (right):

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:16: if
isinstance(path_or_paths, list) or isinstance(path_or_paths, set):
On 2013/05/11 20:39:53, kalman wrote:
> isinstance(path_or_paths, (tuple, list, set))
> 
> that said, it's nicer for functions to accept a single type and either
> - this method can only take a sequence, and _ToObjectStoreValue does
> _MakeKey([key])[0] - in which case call this MakeKeys.
> - this takes a single value and where it's used below does a list
comprehension

Done.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:24: return
{key[0:key.find('@')]: _HandleBinary(raw_value[key], binary)
On 2013/05/11 20:39:53, kalman wrote:
> What happens if a patchset contains both binary and non-binary data? Do we
need
> to handle that at all? Can it happen?

For this reason, CachingRietveldPatcher always read from RietveldPatcher with
binary=True and cache raw data. When returning the data, it is converted to
unicode if binary=False.

It can't happen for now. But if in the future, Rietveld can handle binary files,
this should also work.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:25: for key in
raw_value}
On 2013/05/11 20:39:53, kalman wrote:
> btw @ is a legitimate character to appear in a file path, should be
rfind('@').

Maybe use a character that's not legitimate in file path here? (but I believe no
one will ever commit a file with '@' in its name.)

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:63: class
assumes that applying to all patched files one at a time is more
On 2013/05/11 20:39:53, kalman wrote:
> s/one at a time/at once/

Done.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:79: value =
self._list_object_store.Get(key).Get()
On 2013/05/11 20:39:53, kalman wrote:
> Re-using list_object_store here and in GetPatchedFiles is a bit confusing.
> Having an object store where you only ever access a single key is fine,
> version_object_store?

Done.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:90: def
GetPatchedFiles(self, version = None):
On 2013/05/11 20:39:53, kalman wrote:
> version=None

Done.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:90: def
GetPatchedFiles(self, version = None):
On 2013/05/11 20:39:53, kalman wrote:
> version=None

Done.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:97: value =
self._patcher.GetPatchedFiles(version)
On 2013/05/11 20:39:53, kalman wrote:
> more descriptive name than "value" pls

Done.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/caching_rietveld_patcher.py:119: version),
On 2013/05/11 20:39:53, kalman wrote:
> why is binary explicitly True?

See the comment above. Maybe I should add a comment here.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
File chrome/common/extensions/docs/server2/caching_rietveld_patcher_test.py
(right):

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/caching_rietveld_patcher_test.py:15: class
TestPatcher(Patcher):
On 2013/05/11 20:39:53, kalman wrote:
> pull this into its own file. maybe a class in the future will want one.

Done.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
File chrome/common/extensions/docs/server2/chained_compiled_file_system.py
(right):

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:8: class
ChainedCompiledFileSystem(CompiledFileSystem):
On 2013/05/11 20:39:53, kalman wrote:
> comment

Done.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:8: class
ChainedCompiledFileSystem(CompiledFileSystem):
On 2013/05/11 20:39:53, kalman wrote:
> pls always use composition not inheritance - particularly in python, in which
> object graphs are confusing enough already.
> 
> the only exception to this is interfaces/mixins like
> FileSystem/ObjectStore/Patcher/etc are. You will notice there is no case of
> implementation inheritance in the docserver, except where enforced by
libraries.
> 
> What I imagine this class might look like:
> 
> class ChainedCompiledFileSystem(object):
>   # Comment like...
>   '''A CompiledFileSystem implementation which gets its data from a chain of
> delegate
>   CompiledFileSystems. ... the reason why you need it...
>   '''
>   def Factory(object):
>     def __init__(self, compiled_fs_factory_chain):
>       self._compiled_fs_factory_chain = compiled_fs_factory_chain
> 
>     def Create(self, populate_function, cls, category=None):
>       compiled_fs_chain = [factory(populate_function, cls, category)
>                            for factory in self._compiled_fs_factory_chain]
>       return ChainedCompiledFileSystem(compiled_fs_chain)
> 
>   def __init__(self, compiled_fs_chain):
>     self._compiled_fs_chain = compiled_fs_chain
> 
>   def GetFromFile(self, path, binary=False):
>     for compiled_fs in self._compiled_fs_chain:
>       try:
>         return compiled_fs.GetFromFile(path, binary)
>       except FileNotFoundError:
>         continue
>     # For stack trace.
>     self._compiled_fs_chain[-1].GetFromFile(path, binary)
>     raise AssertionError()
> 
>   def GetFromFileListing(self, path):
>     ...
> 
> which does away with file_system - but from what I can tell, if the chain
passed
> into this file has the patched file system first, it will get that version
> first?

Done.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
File chrome/common/extensions/docs/server2/chained_compiled_file_system_test.py
(right):

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/chained_compiled_file_system_test.py:68: 
On 2013/05/11 20:39:53, kalman wrote:
> test binary here as well?

But ChainedCompiledFileSystem does not do anything related to binary. It just
passes it to CompiledFileSystem.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
File chrome/common/extensions/docs/server2/compiled_file_system.py (right):

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/compiled_file_system.py:16: def
__init__(self, file_system, object_store_creator):
On 2013/05/11 20:39:53, kalman wrote:
> See comments in CacheChainFileSystem. I don't think this file really needs to
> change. 

No. Factory doesn't need to be changed. But there must be some way to expose
CacheEntry so that ChainedCompiledFileSystem knows the version of the data read.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
File chrome/common/extensions/docs/server2/patch_servlet.py (right):

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/patch_servlet.py:69: return
Response.NotFound('Malformed URL.')
On 2013/05/11 20:39:53, kalman wrote:
> Maybe print what the URL should look like

Done.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/patch_servlet.py:74: self._delegate =
PatchServlet.Delegate(issue)
On 2013/05/11 20:39:53, kalman wrote:
> everything above here can go in the constructor.

but what about return Response.NotFound('Malformed URL.')?

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/patch_servlet.py:86: if
response.IsRedirect():
On 2013/05/11 20:39:53, kalman wrote:
> maybe this should actually be GetRedirect().
> 
> redirect = response.GetRedirect()
> if redirect is not None:
>   ...

Done.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
File chrome/common/extensions/docs/server2/patch_servlet_test.py (right):

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/patch_servlet_test.py:34: def
renderWithPatch(self, path, issue):
On 2013/05/11 20:39:53, kalman wrote:
> _RenderWithPatch
> 
> though it looks like none of these needs to be instance methods of
> PatchServletTest, they can be outside.

Done.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/patch_servlet_test.py:39: def
renderWithoutPatch(self, path):
On 2013/05/11 20:39:53, kalman wrote:
> _RenderWithoutPatch

Done.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/patch_servlet_test.py:51: def
renderAndCompare(self, path, issue):
On 2013/05/11 20:39:53, kalman wrote:
> _IsRenderingEqual?

Done.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
File chrome/common/extensions/docs/server2/patcher.py (right):

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/patcher.py:7: ''' Returns (added_files,
deleted_files, modified_files).
On 2013/05/11 20:39:53, kalman wrote:
> Comment what version= means.

Done.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/patcher.py:17: def Apply(self, paths,
file_system, binary, version=None):
On 2013/05/11 20:39:53, kalman wrote:
> I can't see file_system being used anywhere

No. It's not actually used because RietveldPatcher doesn't really do patching
against existing files. It just returns patched files' data.
However, other kind of patchers do need this.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/patcher.py:19: data. Throws
FileNotFoundError if |paths| contains deleted files.
On 2013/05/11 20:39:53, kalman wrote:
> comment what version= means.
> 
> P.S. I was under the impression that codereview has issues with binary files?
> Nevermind.

Yes. And trying to fetch tarball for an issue with binary files results in a
500. But anyway, if it is fixed in the future, it should work with binary files
too.

[not at google - send to devlin, 2013-05-12 03:28:27]

Cool, I see you're going to upload changes to PatchServlet later?

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
File chrome/common/extensions/docs/server2/patch_servlet.py (right):

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/patch_servlet.py:74: self._delegate =
PatchServlet.Delegate(issue)
On 2013/05/12 03:01:47, 方觉(Fang Jue) wrote:
> On 2013/05/11 20:39:53, kalman wrote:
> > everything above here can go in the constructor.
> 
> but what about return Response.NotFound('Malformed URL.')?

I see. Ok. It's just weird overriding a field on self that was set on
construction.

What CronServlet does it has a delegate_for_test. You could actually call the
field that to be clear. Then here,

delegate = self._delegate_for_test or PathServlet.Delegate(issue)

https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/chained_compiled_file_system.py
(right):

https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:20: are
slightly different. This makes it possible to reuse cached compiled data
Nice comment, thanks.

But - s/two compiled file systems/compiled file systems/

(no "two")

https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:43:
cache_entry = compiled_fs._GetCacheEntryFromFile(path, binary)
Is the version stuff necessary if you check them in the other order, and
implement that thing in PatchedFileSystem where you raise a
FileNotFoundException that I mentioned?

If it's *really* necessary to do this then you should inject file_system (like
you had before - I was operating under the assumption in the last paragraph) and
actually exposed a GetCachedVersion or something method on CompiledFileSystem.

https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:48: return
self._compiled_fs_chain[0].GetFromFile(path, binary)
Coud you add the comment here and below "Try first operation again to generate
the correct stack trace".

https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/patch_servlet.py (right):

https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
chrome/common/extensions/docs/server2/patch_servlet.py:8: from
chained_compiled_file_system import (CompiledFileSystem,
CompiledFIleSystem?

[方觉(Fang Jue), 2013-05-12 04:22:06]

> Cool, I see you're going to upload changes to PatchServlet later?
Actually, it was waiting for presubmit when the previous message is sent. (and
it's just time for lunch.) Changes are uploaded now.

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
File chrome/common/extensions/docs/server2/patch_servlet_test.py (right):

https://codereview.chromium.org/14125010/diff/177004/chrome/common/extensions...
chrome/common/extensions/docs/server2/patch_servlet_test.py:42: def
compare(self, patched_response, unpatched_response, issue):
On 2013/05/11 20:39:53, kalman wrote:
> _Compare, but besides "compare" usually implies a < operation not an ==. I'd
> just inline it within RenderAndCompare, particularly given you have the
> opportunity to be more descriptive then, namely, return a (bool, message)
tuple
> then when using assertTrue below, do assertTrue(*_IsRenderingEqual(...)) so
that
> it says what went wrong.

Done.

https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/chained_compiled_file_system.py
(right):

https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:20: are
slightly different. This makes it possible to reuse cached compiled data
On 2013/05/12 03:28:27, kalman wrote:
> Nice comment, thanks.
> 
> But - s/two compiled file systems/compiled file systems/
> 
> (no "two")

Done.

https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:43:
cache_entry = compiled_fs._GetCacheEntryFromFile(path, binary)
On 2013/05/12 03:28:27, kalman wrote:
> Is the version stuff necessary if you check them in the other order, and
> implement that thing in PatchedFileSystem where you raise a
> FileNotFoundException that I mentioned?
> 
Yes. It's necessary. version is used to determine whether the file in
compiled_fs_chain[0] is different from that in other compiled fs. If it is the
same, data cached in other compiled fs can be reused. Otherwise, it has to be
read from compiled_fs_chain[0].

If checked in the other order (I suppose you mean from the first to the last?),
why is this class necessary instead of directly using the first compiled fs?

And RietveldPatcher.GetVersion does not return None now (it throws an exception
instead). Maybe add an assert version is not None in PatchedFileSystem?

> If it's *really* necessary to do this then you should inject file_system (like
> you had before - I was operating under the assumption in the last paragraph)
and
> actually exposed a GetCachedVersion or something method on CompiledFileSystem.
If I inject file_system, I must inject object_store_creator at the same time
(CompiledFileSystem.Factory has two parameters!).

_GetCacheEntryFromFile is almost the same as GetFromFile except for it returns
cache_entry instead of cache_entry.cache_data (we need cache_entry.version). It
probably makes others confused and think that it only returns cached data and
never actually read from file system but I can't find a better name.

https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:48: return
self._compiled_fs_chain[0].GetFromFile(path, binary)
On 2013/05/12 03:28:27, kalman wrote:
> Coud you add the comment here and below "Try first operation again to generate
> the correct stack trace".

Done.

https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/patch_servlet.py (right):

https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
chrome/common/extensions/docs/server2/patch_servlet.py:8: from
chained_compiled_file_system import (CompiledFileSystem,
On 2013/05/12 03:28:27, kalman wrote:
> CompiledFIleSystem?

oh, sorry.

[方觉(Fang Jue), 2013-05-12 04:28:13]

crrev.com/14712010 was committed and reverted. What version in app.yaml should I
use now?

[not at google - send to devlin, 2013-05-12 04:49:25]

Sorry about that. Make it 2-2-1 until I fix the way that we can't handle
app.yaml versions being reverted!

[not at google - send to devlin, 2013-05-12 05:11:41]

lgtm after that

https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/chained_compiled_file_system.py
(right):

https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:43:
cache_entry = compiled_fs._GetCacheEntryFromFile(path, binary)

> Yes. It's necessary. version is used to determine whether the file in
> compiled_fs_chain[0] is different from that in other compiled fs. If it is the
> same, data cached in other compiled fs can be reused. Otherwise, it has to be
> read from compiled_fs_chain[0].
> 
> If checked in the other order (I suppose you mean from the first to the
last?),
> why is this class necessary instead of directly using the first compiled fs?

Ok, never mind, I found a problem with what I was thinking anyway.

> 
> And RietveldPatcher.GetVersion does not return None now (it throws an
exception
> instead). Maybe add an assert version is not None in PatchedFileSystem?

Sure, not what I meant, but this sounds good.

> If I inject file_system, I must inject object_store_creator at the same time
> (CompiledFileSystem.Factory has two parameters!).

I meant both file system and the compiled file systems separately. That would
mean less magic in querying compiled_fs_chain[0]._file_system which is bad
because (a) assuming something about compiled_fs_chain[0] and (b) accessing
private member of a compiled file system.

> 
> _GetCacheEntryFromFile is almost the same as GetFromFile except for it returns
> cache_entry instead of cache_entry.cache_data (we need cache_entry.version).
It
> probably makes others confused and think that it only returns cached data and
> never actually read from file system but I can't find a better name.

I'm just saying elevate it from private to public if you need to access it. Make
CompiledFileSystem have GetFromFile, GetFromFileListing, and GetVersion ("gets
the version of the data that would be returned from calling either
GetFromFile/GetFromFileListing with |path|").

[not at google - send to devlin, 2013-05-12 05:13:42]

https://codereview.chromium.org/14125010/diff/196035/chrome/common/extensions...
File chrome/common/extensions/docs/server2/chained_compiled_file_system.py
(right):

https://codereview.chromium.org/14125010/diff/196035/chrome/common/extensions...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:42:
version = self._compiled_fs_chain[0]._file_system.Stat(path).version
p.s. however this code ends up turning out, it's quite subtle, it needs some
comments.

[方觉(Fang Jue), 2013-05-12 05:57:46]

On 2013/05/12 05:11:41, kalman wrote:
> lgtm after that
> 
>
https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
> File chrome/common/extensions/docs/server2/chained_compiled_file_system.py
> (right):
> 
>
https://codereview.chromium.org/14125010/diff/200001/chrome/common/extensions...
> chrome/common/extensions/docs/server2/chained_compiled_file_system.py:43:
> cache_entry = compiled_fs._GetCacheEntryFromFile(path, binary)
> 
> > Yes. It's necessary. version is used to determine whether the file in
> > compiled_fs_chain[0] is different from that in other compiled fs. If it is
the
> > same, data cached in other compiled fs can be reused. Otherwise, it has to
be
> > read from compiled_fs_chain[0].
> > 
> > If checked in the other order (I suppose you mean from the first to the
> last?),
> > why is this class necessary instead of directly using the first compiled fs?
> 
> Ok, never mind, I found a problem with what I was thinking anyway.
> 
> > 
> > And RietveldPatcher.GetVersion does not return None now (it throws an
> exception
> > instead). Maybe add an assert version is not None in PatchedFileSystem?
> 
> Sure, not what I meant, but this sounds good.
> 
> > If I inject file_system, I must inject object_store_creator at the same time
> > (CompiledFileSystem.Factory has two parameters!).
> 
> I meant both file system and the compiled file systems separately. That would
> mean less magic in querying compiled_fs_chain[0]._file_system which is bad
> because (a) assuming something about compiled_fs_chain[0] and (b) accessing
> private member of a compiled file system.
> 

Done.

> > 
> > _GetCacheEntryFromFile is almost the same as GetFromFile except for it
returns
> > cache_entry instead of cache_entry.cache_data (we need cache_entry.version).
> It
> > probably makes others confused and think that it only returns cached data
and
> > never actually read from file system but I can't find a better name.
> 
> I'm just saying elevate it from private to public if you need to access it.
Make
> CompiledFileSystem have GetFromFile, GetFromFileListing, and GetVersion ("gets
> the version of the data that would be returned from calling either
> GetFromFile/GetFromFileListing with |path|").

Reverted previous changes and added StatFile and StatFileListing.

[not at google - send to devlin, 2013-05-12 06:18:31]

lgtm

[commit-bot: I haz the power, 2013-05-12 07:33:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fangjue23303@gmail.com/14125010/180006

[commit-bot: I haz the power, 2013-05-13 04:40:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fangjue23303@gmail.com/14125010/180006

[commit-bot: I haz the power, 2013-05-13 04:57:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fangjue23303@gmail.com/14125010/215002

[方觉(Fang Jue), 2013-05-13 05:14:54]

On 2013/05/13 04:57:03, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>
https://chromium-status.appspot.com/cq/fangjue23303%40gmail.com/14125010/215002

All win bots failed with the following message (apply_issue):

Failed to apply patch for
chrome/common/extensions/docs/server2/caching_rietveld_patcher_test.py:
While running svn propset svn:executable *
chrome/common/extensions/docs/server2/caching_rietveld_patcher_test.py
--non-interactive;
  patching file
chrome/common/extensions/docs/server2/caching_rietveld_patcher_test.py
  A        
chrome\common\extensions\docs\server2\caching_rietveld_patcher_test.py
  property 'svn:eol-style' set on
'chrome\common\extensions\docs\server2\caching_rietveld_patcher_test.py'
  Property svn:eol-style=LF
  svn: Skipping argument: '.svn' ends in a reserved name
  property 'svn:executable' set on '.gitignore'
  property 'svn:executable' set on '.gitmodules'
  property 'svn:executable' set on 'Android.mk'
  svn: Cannot set 'svn:executable' on a directory ('android_webview')

Why did it try to set svn:executable on files in src/?

[not at google - send to devlin, 2013-05-13 05:32:21]

It might not have been your patch?

[方觉(Fang Jue), 2013-05-13 05:35:37]

On 2013/05/13 05:32:21, kalman wrote:
> It might not have been your patch?

After some searching, it seems that '*' in the command line was expanded to all
files in the current directory (src/) on Windows.

[not at google - send to devlin, 2013-05-13 05:35:43]

Looked through commit things, don't see anything, but I don't really know what I
was looking for.

I presume it would set +x on things if your patch asked it to?

[not at google - send to devlin, 2013-05-13 05:36:32]

Ok. I don't know where * came from but cool, glad it's fixed.

[方觉(Fang Jue), 2013-05-13 05:48:27]

On 2013/05/13 05:36:32, kalman wrote:
> Ok. I don't know where * came from but cool, glad it's fixed.

fixed?

[commit-bot: I haz the power, 2013-05-13 08:20:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fangjue23303@gmail.com/14125010/215002

[commit-bot: I haz the power, 2013-05-13 08:34:47]

Retried try job too often on win_x64_rel for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[commit-bot: I haz the power, 2013-05-13 13:54:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fangjue23303@gmail.com/14125010/215002

[方觉(Fang Jue), 2013-05-13 14:00:09]

On 2013/05/13 13:54:42, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>
https://chromium-status.appspot.com/cq/fangjue23303%40gmail.com/14125010/215002

Still failed. Can I add a NOTRY=TRUE?

[not at google - send to devlin, 2013-05-13 14:15:13]

Committed patchset #50 manually as r199727 (presubmit successful).