Extensions Docs Server: Efficient MemcacheFileSystem

Extensions Docs Server: Efficient MemcacheFileSystem

MemcacheFileSystem will now use viewvc for statting, and will stat all the
children of a directory.

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

[cduvall, 2012-08-09 20:56:17]

I wanted you to look at this CL, just to make sure I'm on the right track. It
still needs tests.

[not at google - send to devlin, 2012-08-10 06:42:24]

yep, looking good.

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

http://codereview.chromium.org/10828235/diff/2001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/file_system.py:45: of the directory.
This would be a strange interface.

If a file, it should return a StatInfo for the file. An implementation might end
up getting the stat for the whole directory the pulling just that single file
out (inefficient but whatever).

(If callers want the directory's stat, they should figure out what the directory
is then Stat that).

If a directory... what you've written is correct.

http://codereview.chromium.org/10828235/diff/2001/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/handler.py (right):

http://codereview.chromium.org/10828235/diff/2001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/handler.py:66: svn_url =
_GetURLFromBranch(branch) + '/' + EXTENSIONS_PATH
nice

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

http://codereview.chromium.org/10828235/diff/2001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/local_file_system.py:50: for filename in
os.listdir(path)]
can construct the dict without an intermediate list:

versions = dict((filename, os.stat(...))
                for filename in os.listdir(path))

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

http://codereview.chromium.org/10828235/diff/2001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_file_system.py:38: # was not
the prettiest.
heh. Let's hope it doesn't change; but if it does, make sure we can catch it and
update this function + redeploy.

http://codereview.chromium.org/10828235/diff/2001/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/subversion_file_system_test.py
(right):

http://codereview.chromium.org/10828235/diff/2001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_file_system_test.py:37: #
self.assertEqual(0, self._file_system.Stat('list/dir/').version)
yeah, you might want to download
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/api/
and test that directly.

[not at google - send to devlin, 2012-08-10 06:42:47]

(that was easier than I thought it would be. nice)

[cduvall, 2012-08-11 00:15:23]

Tests are in, this ones ready.

https://chromiumcodereview.appspot.com/10828235/diff/2001/chrome/common/exten...
File chrome/common/extensions/docs/server2/file_system.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/file_system.py:45: of the directory.
On 2012/08/10 06:42:24, kalman wrote:
> This would be a strange interface.
> 
> If a file, it should return a StatInfo for the file. An implementation might
end
> up getting the stat for the whole directory the pulling just that single file
> out (inefficient but whatever).
> 
> (If callers want the directory's stat, they should figure out what the
directory
> is then Stat that).
> 
> If a directory... what you've written is correct.

Done.

https://chromiumcodereview.appspot.com/10828235/diff/2001/chrome/common/exten...
File chrome/common/extensions/docs/server2/local_file_system.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/local_file_system.py:50: for filename in
os.listdir(path)]
On 2012/08/10 06:42:24, kalman wrote:
> can construct the dict without an intermediate list:
> 
> versions = dict((filename, os.stat(...))
>                 for filename in os.listdir(path))

Done.

https://chromiumcodereview.appspot.com/10828235/diff/2001/chrome/common/exten...
File chrome/common/extensions/docs/server2/subversion_file_system_test.py
(right):

https://chromiumcodereview.appspot.com/10828235/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/subversion_file_system_test.py:37: #
self.assertEqual(0, self._file_system.Stat('list/dir/').version)
On 2012/08/10 06:42:24, kalman wrote:
> yeah, you might want to download
> http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/api/
> and test that directly.

Done.

[not at google - send to devlin, 2012-08-13 05:34:15]

sweet

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
File chrome/common/extensions/docs/server2/fake_url_fetcher.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/fake_url_fetcher.py:25: return
self._ReadFile(directory[:-1])
How can _ListDir be passed something that isn't a directory? Would that not be a
bug in the calling code? Should that test be done before this call reaches a
non-directory?

If it's legitimate then it at least needs a comment.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
File chrome/common/extensions/docs/server2/local_file_system.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/local_file_system.py:64:
os.stat(os.path.join(self._base_path, path)).st_mtime, {})
This logic is basically repeated in _CreateStatInfo, so put in there? This
method:

return self._CreateStatInfo(...)

above:

def _CreateStatInfo(self, path):
  def Mtime(path):
    return os.stat(path).st_mtime

  if path.endswith('/'):
    versions = dict((filename, Mtime(os.path.join(path, filename))
                     for filename in os.listdir(path))
  else:
    # note to cduvall: prefer None to {} here.
    versions = None
  return self.StatInfo(Mtime(path), versions)

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
File chrome/common/extensions/docs/server2/memcache_file_system.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system.py:31: stat_info =
self.StatInfo(version, {})
memcache can store arbitrary objects, I believe. If you store the StatInfo
object in memcache rather than just the version then you won't need to construct
this fake (and incorrect) {} object.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
File chrome/common/extensions/docs/server2/memcache_file_system_test.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:20:
self.stat_fetch_tracker = False
what's the difference between fetch_tracker and stat_fetch_tracker? seems like
it should be read_fetch_tracker vs stat_fetch_tracker.

That said, a more common idiom would be to count calls to the methods, providing
a Reset method. Like read_counter and stat_counter.

Another idiom I've seen (and quite like) is combining the resetting with the
querying. That reduces the chance of forgetting to call Reset and also the
verbosity of the tests.

So like, make read/stat counter private, then have

def CheckAndReset(self, read_count=0, stat_count=0):
  try:
    return (self._read_count == read_count and
            self._stat_count == stat_count)
  finally:
    self._read_count = 0
    self._stat_count = 0

then tests can call

self.assertTrue(fake_fs.CheckAndReset(read_count=1, stat_count=0))

etc.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:29: if paths:
Is this so that if read is called with an empty object, it doesn't trigger
fetched?

Sounds fishy. Why does it happen? We should fix the calling classes.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:31: return
Future(value= dict((path, path) for path in paths))
nit: extra space after value=

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:40: def
_SetCacheItem(self, key, value, stat):
would prefer this be SetReadCacheItem since there's SetStatCacheItem (ditto
delete).

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:75:
self._file_system = MemcacheFileSystem(fake_fs, self._memcache)
if you find yourself doing this, sounds like _file_system shouldn't be a member
variable.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:80: # Test
resource is not re-fetched.
"Resource has been memcached, so test resource is not re-fetched."

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
File chrome/common/extensions/docs/server2/subversion_file_system.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/subversion_file_system.py:27: return
xml.parseString(html)
Wow, it kinda-works with an XML parser? Amazing.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/subversion_file_system.py:33: return
self._ParseHTML('\n'.join(new_html))
I love a good list comprehension challenge.

return self._ParseHTML('\n'.join(
    line for (i, line) in enumerate(html.split('\n'))
    if e.lineno != i + 1))

or something like that.

Removing the offending line, though... interesting approach. I'd fear it being a
bit flaky (e.g. curious, but why didn't you choose to use one of the HTML
parsing libraries?) but let's see.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/subversion_file_system.py:38: # was not
the prettiest.
yup

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/subversion_file_system.py:52: for i in
range(len(a_list)):
is there something special about a_list that means you can't just iterate over
it using "for i, a in enumerate(a_list)"

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/subversion_file_system.py:55: rev =
a_list[i + 1].getElementsByTagName('strong')[0]
so we know for sure that this won't be past the end of the list?

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/subversion_file_system.py:71:
stat_info.version = stat_info.child_versions[filename]
throw FileNotFoundException if it isn't?

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
File
chrome/common/extensions/docs/server2/test_data/file_system/stat_result.json
(right):

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/test_data/file_system/stat_result.json:2:
"extension_api.h": "146163",
you've eyeballed this to be correct, right?

[cduvall, 2012-08-13 19:45:44]

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
File chrome/common/extensions/docs/server2/fake_url_fetcher.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/fake_url_fetcher.py:25: return
self._ReadFile(directory[:-1])
On 2012/08/13 05:34:15, kalman wrote:
> How can _ListDir be passed something that isn't a directory? Would that not be
a
> bug in the calling code? Should that test be done before this call reaches a
> non-directory?
> 
> If it's legitimate then it at least needs a comment.

This is just for testing. The file has the contents of what a fetch from SVN
returns from fetching a directory, so technically it is listing a directory.
Added comment though.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
File chrome/common/extensions/docs/server2/local_file_system.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/local_file_system.py:64:
os.stat(os.path.join(self._base_path, path)).st_mtime, {})
On 2012/08/13 05:34:15, kalman wrote:
> This logic is basically repeated in _CreateStatInfo, so put in there? This
> method:
> 
> return self._CreateStatInfo(...)
> 
> above:
> 
> def _CreateStatInfo(self, path):
>   def Mtime(path):
>     return os.stat(path).st_mtime
> 
>   if path.endswith('/'):
>     versions = dict((filename, Mtime(os.path.join(path, filename))
>                      for filename in os.listdir(path))
>   else:
>     # note to cduvall: prefer None to {} here.
>     versions = None
>   return self.StatInfo(Mtime(path), versions)

Done.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
File chrome/common/extensions/docs/server2/memcache_file_system.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system.py:31: stat_info =
self.StatInfo(version, {})
On 2012/08/13 05:34:15, kalman wrote:
> memcache can store arbitrary objects, I believe. If you store the StatInfo
> object in memcache rather than just the version then you won't need to
construct
> this fake (and incorrect) {} object.

Memcache can store dictionaries, but not objects. They need to have a pickle
method I believe. I changed this to use None instead of {}, which I think is a
little less confusing.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
File chrome/common/extensions/docs/server2/memcache_file_system_test.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:20:
self.stat_fetch_tracker = False
On 2012/08/13 05:34:15, kalman wrote:
> what's the difference between fetch_tracker and stat_fetch_tracker? seems like
> it should be read_fetch_tracker vs stat_fetch_tracker.
> 
> That said, a more common idiom would be to count calls to the methods,
providing
> a Reset method. Like read_counter and stat_counter.
> 
> Another idiom I've seen (and quite like) is combining the resetting with the
> querying. That reduces the chance of forgetting to call Reset and also the
> verbosity of the tests.
> 
> So like, make read/stat counter private, then have
> 
> def CheckAndReset(self, read_count=0, stat_count=0):
>   try:
>     return (self._read_count == read_count and
>             self._stat_count == stat_count)
>   finally:
>     self._read_count = 0
>     self._stat_count = 0
> 
> then tests can call
> 
> self.assertTrue(fake_fs.CheckAndReset(read_count=1, stat_count=0))
> 
> etc.

Done.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:29: if paths:
On 2012/08/13 05:34:15, kalman wrote:
> Is this so that if read is called with an empty object, it doesn't trigger
> fetched?
> 
> Sounds fishy. Why does it happen? We should fix the calling classes.

Done.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:31: return
Future(value= dict((path, path) for path in paths))
On 2012/08/13 05:34:15, kalman wrote:
> nit: extra space after value=

Done.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:40: def
_SetCacheItem(self, key, value, stat):
On 2012/08/13 05:34:15, kalman wrote:
> would prefer this be SetReadCacheItem since there's SetStatCacheItem (ditto
> delete).

Done.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:75:
self._file_system = MemcacheFileSystem(fake_fs, self._memcache)
On 2012/08/13 05:34:15, kalman wrote:
> if you find yourself doing this, sounds like _file_system shouldn't be a
member
> variable.

Done.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system_test.py:80: # Test
resource is not re-fetched.
On 2012/08/13 05:34:15, kalman wrote:
> "Resource has been memcached, so test resource is not re-fetched."

Done.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
File chrome/common/extensions/docs/server2/subversion_file_system.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/subversion_file_system.py:33: return
self._ParseHTML('\n'.join(new_html))
On 2012/08/13 05:34:15, kalman wrote:
> I love a good list comprehension challenge.
> 
> return self._ParseHTML('\n'.join(
>     line for (i, line) in enumerate(html.split('\n'))
>     if e.lineno != i + 1))
> 
> or something like that.
> 
> Removing the offending line, though... interesting approach. I'd fear it being
a
> bit flaky (e.g. curious, but why didn't you choose to use one of the HTML
> parsing libraries?) but let's see.

I didn't use one of the HTML parsing libraries because the default python
libraries (that I found) required a lot more code to find and remove mismatched
tags, so I thought this would probably be better for this situation. There are
third party libraries like Beautiful Soup which handle this, but I didn't think
it was worth adding a third_party library.

Also, I looked at the HTML, and found the problem was a stray tag on a line. If
this HTML changes we're going to have to change this a lot anyway, but I think
it's pretty stable.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/subversion_file_system.py:52: for i in
range(len(a_list)):
On 2012/08/13 05:34:15, kalman wrote:
> is there something special about a_list that means you can't just iterate over
> it using "for i, a in enumerate(a_list)"

No, no there isn't. That's just me being strange.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/subversion_file_system.py:55: rev =
a_list[i + 1].getElementsByTagName('strong')[0]
On 2012/08/13 05:34:15, kalman wrote:
> so we know for sure that this won't be past the end of the list?

If the HTML doesn't change it won't go past the end of the list. I'll put this
in a try just in case though.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/subversion_file_system.py:71:
stat_info.version = stat_info.child_versions[filename]
On 2012/08/13 05:34:15, kalman wrote:
> throw FileNotFoundException if it isn't?

Done.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
File
chrome/common/extensions/docs/server2/test_data/file_system/stat_result.json
(right):

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/test_data/file_system/stat_result.json:2:
"extension_api.h": "146163",
On 2012/08/13 05:34:15, kalman wrote:
> you've eyeballed this to be correct, right?

Yep.

[not at google - send to devlin, 2012-08-13 23:02:14]

lgtm but please look at the pickle thing.

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

http://codereview.chromium.org/10828235/diff/5002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:31: stat_info =
self.StatInfo(version, {})
On 2012/08/13 19:45:45, cduvall wrote:
> On 2012/08/13 05:34:15, kalman wrote:
> > memcache can store arbitrary objects, I believe. If you store the StatInfo
> > object in memcache rather than just the version then you won't need to
> construct
> > this fake (and incorrect) {} object.
> 
> Memcache can store dictionaries, but not objects. They need to have a pickle
> method I believe. I changed this to use None instead of {}, which I think is a
> little less confusing.

I just checked and it looks like pickle can handle at least simple objects (I
tested objects with simple and list fields). So... let's at least try it.

Here is my output from the python console, by the way:

> python
Python 2.7.3 (default, Apr 13 2012, 00:05:08) 
[GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class Foo(object):
...   def __init__(self, x, xs):
...     self.x = x
...     self.xs = xs
... 
>>> f1 = Foo(10, [1,2,3])
>>> import pickle
>>> f2 = pickle.loads(pickle.dumps(f1))
>>> f1 == f2
False
>>> f2.x
10
>>> f2.xs
[1, 2, 3]
>>>

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

http://codereview.chromium.org/10828235/diff/5002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_file_system.py:33: return
self._ParseHTML('\n'.join(new_html))
On 2012/08/13 19:45:45, cduvall wrote:
> On 2012/08/13 05:34:15, kalman wrote:
> > I love a good list comprehension challenge.
> > 
> > return self._ParseHTML('\n'.join(
> >     line for (i, line) in enumerate(html.split('\n'))
> >     if e.lineno != i + 1))
> > 
> > or something like that.
> > 
> > Removing the offending line, though... interesting approach. I'd fear it
being
> a
> > bit flaky (e.g. curious, but why didn't you choose to use one of the HTML
> > parsing libraries?) but let's see.
> 
> I didn't use one of the HTML parsing libraries because the default python
> libraries (that I found) required a lot more code to find and remove
mismatched
> tags, so I thought this would probably be better for this situation. There are
> third party libraries like Beautiful Soup which handle this, but I didn't
think
> it was worth adding a third_party library.
> 
> Also, I looked at the HTML, and found the problem was a stray tag on a line.
If
> this HTML changes we're going to have to change this a lot anyway, but I think
> it's pretty stable.

No list comprehension? :(

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

http://codereview.chromium.org/10828235/diff/1013/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/fake_url_fetcher.py:25: # from SVN. This
reads an HTML file that has the directories HTML.
Ah

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

http://codereview.chromium.org/10828235/diff/1013/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:54: new_items =
self._file_system.Read(uncached, binary=binary).Get()
Add a TODO here,

TODO(cduvall): if there are uncached items we should return an asynchronous
future.

there might be an appropriate bug filed about that somewhere. Seems kind of
important to fix actually, since this would prevent us being able to download
things in parallel.

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

http://codereview.chromium.org/10828235/diff/1013/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_file_system.py:56: break
Exceptions aren't a good way of signaling control flow.

if i + 1 < len(a_list):
  break

would be better.

[not at google - send to devlin, 2012-08-14 04:00:08]

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

http://codereview.chromium.org/10828235/diff/1013/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_file_system.py:56: break
On 2012/08/13 23:02:14, kalman wrote:
> Exceptions aren't a good way of signaling control flow.
> 
> if i + 1 < len(a_list):
>   break
> 
> would be better.

and by that I mean >= not <

[cduvall, 2012-08-14 18:15:00]

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
File chrome/common/extensions/docs/server2/memcache_file_system.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system.py:31: stat_info =
self.StatInfo(version, {})
On 2012/08/13 23:02:14, kalman wrote:
> On 2012/08/13 19:45:45, cduvall wrote:
> > On 2012/08/13 05:34:15, kalman wrote:
> > > memcache can store arbitrary objects, I believe. If you store the StatInfo
> > > object in memcache rather than just the version then you won't need to
> > construct
> > > this fake (and incorrect) {} object.
> > 
> > Memcache can store dictionaries, but not objects. They need to have a pickle
> > method I believe. I changed this to use None instead of {}, which I think is
a
> > little less confusing.
> 
> I just checked and it looks like pickle can handle at least simple objects (I
> tested objects with simple and list fields). So... let's at least try it.
> 
> Here is my output from the python console, by the way:
> 
> > python
> Python 2.7.3 (default, Apr 13 2012, 00:05:08) 
> [GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin
> Type "help", "copyright", "credits" or "license" for more information.
> >>> class Foo(object):
> ...   def __init__(self, x, xs):
> ...     self.x = x
> ...     self.xs = xs
> ... 
> >>> f1 = Foo(10, [1,2,3])
> >>> import pickle
> >>> f2 = pickle.loads(pickle.dumps(f1))
> >>> f1 == f2
> False
> >>> f2.x
> 10
> >>> f2.xs
> [1, 2, 3]
> >>> 

Aha! The problem with pickle was nested classes. It can't pickle a nested class.
Here's my output:
"PicklingError: Can't pickle <class 'file_system.StatInfo'>: attribute lookup
file_system.StatInfo failed"

I'm going to leave this just storing the version for now, and I can change it
later if we want.

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
File chrome/common/extensions/docs/server2/subversion_file_system.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/subversion_file_system.py:33: return
self._ParseHTML('\n'.join(new_html))
On 2012/08/13 23:02:14, kalman wrote:
> On 2012/08/13 19:45:45, cduvall wrote:
> > On 2012/08/13 05:34:15, kalman wrote:
> > > I love a good list comprehension challenge.
> > > 
> > > return self._ParseHTML('\n'.join(
> > >     line for (i, line) in enumerate(html.split('\n'))
> > >     if e.lineno != i + 1))
> > > 
> > > or something like that.
> > > 
> > > Removing the offending line, though... interesting approach. I'd fear it
> being
> > a
> > > bit flaky (e.g. curious, but why didn't you choose to use one of the HTML
> > > parsing libraries?) but let's see.
> > 
> > I didn't use one of the HTML parsing libraries because the default python
> > libraries (that I found) required a lot more code to find and remove
> mismatched
> > tags, so I thought this would probably be better for this situation. There
are
> > third party libraries like Beautiful Soup which handle this, but I didn't
> think
> > it was worth adding a third_party library.
> > 
> > Also, I looked at the HTML, and found the problem was a stray tag on a line.
> If
> > this HTML changes we're going to have to change this a lot anyway, but I
think
> > it's pretty stable.
> 
> No list comprehension? :(

Oops forgot to change it! Done :)

https://chromiumcodereview.appspot.com/10828235/diff/1013/chrome/common/exten...
File chrome/common/extensions/docs/server2/memcache_file_system.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/1013/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system.py:54: new_items =
self._file_system.Read(uncached, binary=binary).Get()
On 2012/08/13 23:02:14, kalman wrote:
> Add a TODO here,
> 
> TODO(cduvall): if there are uncached items we should return an asynchronous
> future.
> 
> there might be an appropriate bug filed about that somewhere. Seems kind of
> important to fix actually, since this would prevent us being able to download
> things in parallel.

Done.

https://chromiumcodereview.appspot.com/10828235/diff/1013/chrome/common/exten...
File chrome/common/extensions/docs/server2/subversion_file_system.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/1013/chrome/common/exten...
chrome/common/extensions/docs/server2/subversion_file_system.py:56: break
On 2012/08/14 04:00:08, kalman wrote:
> On 2012/08/13 23:02:14, kalman wrote:
> > Exceptions aren't a good way of signaling control flow.
> > 
> > if i + 1 < len(a_list):
> >   break
> > 
> > would be better.
> 
> and by that I mean >= not <

Done.

[not at google - send to devlin, 2012-08-15 05:24:05]

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
File chrome/common/extensions/docs/server2/memcache_file_system.py (right):

https://chromiumcodereview.appspot.com/10828235/diff/5002/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system.py:31: stat_info =
self.StatInfo(version, {})
On 2012/08/14 18:15:00, cduvall wrote:
> On 2012/08/13 23:02:14, kalman wrote:
> > On 2012/08/13 19:45:45, cduvall wrote:
> > > On 2012/08/13 05:34:15, kalman wrote:
> > > > memcache can store arbitrary objects, I believe. If you store the
StatInfo
> > > > object in memcache rather than just the version then you won't need to
> > > construct
> > > > this fake (and incorrect) {} object.
> > > 
> > > Memcache can store dictionaries, but not objects. They need to have a
pickle
> > > method I believe. I changed this to use None instead of {}, which I think
is
> a
> > > little less confusing.
> > 
> > I just checked and it looks like pickle can handle at least simple objects
(I
> > tested objects with simple and list fields). So... let's at least try it.
> > 
> > Here is my output from the python console, by the way:
> > 
> > > python
> > Python 2.7.3 (default, Apr 13 2012, 00:05:08) 
> > [GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin
> > Type "help", "copyright", "credits" or "license" for more information.
> > >>> class Foo(object):
> > ...   def __init__(self, x, xs):
> > ...     self.x = x
> > ...     self.xs = xs
> > ... 
> > >>> f1 = Foo(10, [1,2,3])
> > >>> import pickle
> > >>> f2 = pickle.loads(pickle.dumps(f1))
> > >>> f1 == f2
> > False
> > >>> f2.x
> > 10
> > >>> f2.xs
> > [1, 2, 3]
> > >>> 
> 
> Aha! The problem with pickle was nested classes. It can't pickle a nested
class.
> Here's my output:
> "PicklingError: Can't pickle <class 'file_system.StatInfo'>: attribute lookup
> file_system.StatInfo failed"
> 
> I'm going to leave this just storing the version for now, and I can change it
> later if we want.

Ok, let's un-nest StatInfo then (or figure out how to make pickle work).