Docserver: Persist stat cache for versioned file systems

Docserver: Persist stat cache for versioned file systems

This encompasses the following changes:
  * FileSystems can now be versioned
  * GitilesFileSystem is versioned by commit ID
  * CachingFileSystem will use a persistent, version-keyed
    stat cache for versioned file systems.

This CL also corrects refresh cycle synchronization so
the "master" commit ID used to identify the live instance
cache is only updated once all associated refresh tasks
have completed.

BUG=415309
R=kalman@chromium.org
NOTRY=True

Committed: https://crrev.com/360062747ecb8079f2c7467bc5c2fef914e1beb9
Cr-Commit-Position: refs/heads/master@{#301236}

[Ken Rockot(use gerrit already), 2014-10-17 23:27:31]

Here it is - it still needs some work.

This breaks a few tests due to persistent stat caching, which makes me realize
some flaws in that approach. It's still much more performant than what's
happening now, and we won't be hitting the git server nearly as hard.

[not at google - send to devlin, 2014-10-20 21:06:58]

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

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/appengine_url_fetcher.py:45: deadline=40,
Do we know this is failing?

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

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system.py:63: raise
FileNotFoundError('Got 404 when stat\'ing  %s' % path)
It might complicate things for callers - but can we use a different exception
type here, like CacheMissError? Callers can always turn that into
FileNotFoundErrors if they really want.

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/cron.yaml (right):

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/cron.yaml:4: schedule: every 24 hours from
00:00 to 00:00
Ouch.

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

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/handler.py:39: if servlet_name ==
_FORCE_CRON_TARGET:
Why are these servlets special-cased rather than listed in _SERVLETS?

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

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/host_file_system_provider.py:25:
cache_only=False):
Document this parameter (so I know what it means).

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

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/instance_servlet.py:18: def
_GetInstanceMasterCommit():
Comment.

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/instance_servlet.py:20: store =
creator.Create(str, category='commits')
Use InstanceServlet rather than str?

Also you might as well re-use the ObjectStoreCreator that you created in
CreateServerInstance?

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/instance_servlet.py:23: return
store.GetMulti(['master']).Then(get_master).Get()
You could just return store.Get('master').Get()?

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/instance_servlet.py:25: 
dd

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

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/refresh_servlet.py:38: store =
creator.Create(str, category='commits')
Ok - I see why you use str. Maybe you should pull this "master commit thing"
into its own class (or at least own file) which does its own caching.

[Ken Rockot(use gerrit already), 2014-10-22 03:15:09]

Patchset #2 (id:20001) has been deleted

[Ken Rockot(use gerrit already), 2014-10-22 03:19:54]

OK. Overview of new changes:

- FileSystem has a concept of "stable" vs "unstable" identity. This is an
experimental change, LMKWYT. It works pretty well for letting consumers be picky
about how stat info is cached.
- And of course CachingFileSystem can now optionally make its stat info cache
persistent
- RefreshTracker is used to persist state about what tasks have completed vs
what tasks comprise a complete refresh. This lets the final refresh task update
the master commit ID upon completion.
- Cycle time is now 1 hour

In hindsight, I think the meaning of stable and unstable identity should be
reversed, but it made sense the other way when I wrote it. :D

CachingFileSystem will ultimately throw CacheMissErrors, I think (though kinda
weird given that the caching is opaque to fs consumers). Didn't do it yet
because I wanted to push this out.

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

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/appengine_url_fetcher.py:45: deadline=40,
On 2014/10/20 21:06:57, kalman wrote:
> Do we know this is failing?

Yes. I was seeing timeouts occurring at the 20 second mark. I don't know how
consistent it is, but really I also don't see any harm in letting tasks block
for a while on requests.

Alternatively we can turn this way down and let tasks fail when remote services
are unresponsive. This frees up the task queue for other stuff.

If the gitiles delays are really sporadic per request, that might make sense.
Otherwise it would probably just cause the queue to churn a lot.

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

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/caching_file_system.py:63: raise
FileNotFoundError('Got 404 when stat\'ing  %s' % path)
On 2014/10/20 21:06:57, kalman wrote:
> It might complicate things for callers - but can we use a different exception
> type here, like CacheMissError? Callers can always turn that into
> FileNotFoundErrors if they really want.

Will do, but it will take longer to sort that out. Lots of callers do this
special case handling for FileNotFoundError.

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/cron.yaml (right):

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/cron.yaml:4: schedule: every 24 hours from
00:00 to 00:00
On 2014/10/20 21:06:57, kalman wrote:
> Ouch.

Acknowledged.

Bumping down to 1 hour to see how GoB responds.

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

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/handler.py:39: if servlet_name ==
_FORCE_CRON_TARGET:
On 2014/10/20 21:06:57, kalman wrote:
> Why are these servlets special-cased rather than listed in _SERVLETS?

They were meant to be temporary hacks. _force_cron is now useless so I've
removed it. _enqueue is now in _SERVLETS.

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

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/host_file_system_provider.py:25:
cache_only=False):
On 2014/10/20 21:06:57, kalman wrote:
> Document this parameter (so I know what it means).

Done.

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

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/instance_servlet.py:18: def
_GetInstanceMasterCommit():
On 2014/10/20 21:06:57, kalman wrote:
> Comment.

Done.

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/instance_servlet.py:20: store =
creator.Create(str, category='commits')
On 2014/10/20 21:06:57, kalman wrote:
> Use InstanceServlet rather than str?
> 
> Also you might as well re-use the ObjectStoreCreator that you created in
> CreateServerInstance?

Done.

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/instance_servlet.py:23: return
store.GetMulti(['master']).Then(get_master).Get()
On 2014/10/20 21:06:57, kalman wrote:
> You could just return store.Get('master').Get()?

Oh. Derp. Didn't actually look at the base ObjectStore interface. Done.

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

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/refresh_servlet.py:38: store =
creator.Create(str, category='commits')
On 2014/10/20 21:06:58, kalman wrote:
> Ok - I see why you use str. Maybe you should pull this "master commit thing"
> into its own class (or at least own file) which does its own caching.

Done.

[not at google - send to devlin, 2014-10-22 16:48:37]

> (though kinda
> weird given that the caching is opaque to fs consumers).

Before I look at this CL: yep, it would be bad for any CacheMissError to leak
out of the CachingFileSystem. CachingFileSystem should catch these and do what
it wants with them, like turn them into FileSystemErrors I suppose.

Though that said, I will now look at the code to see if my comment makes sense,
because I wouldn't have thought CacheMissErrors would even need to leak out at
all (i.e. that it was all captured in the implementation details of
CachingFileSystem). It may also not make sense for other reasons.

[not at google - send to devlin, 2014-10-22 16:51:58]

One more pre-review comment.

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

https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/appengine_url_fetcher.py:45: deadline=40,
On 2014/10/22 03:19:54, Ken Rockot wrote:
> On 2014/10/20 21:06:57, kalman wrote:
> > Do we know this is failing?
> 
> Yes. I was seeing timeouts occurring at the 20 second mark. I don't know how
> consistent it is, but really I also don't see any harm in letting tasks block
> for a while on requests.
> 
> Alternatively we can turn this way down and let tasks fail when remote
services
> are unresponsive. This frees up the task queue for other stuff.
> 
> If the gitiles delays are really sporadic per request, that might make sense.
> Otherwise it would probably just cause the queue to churn a lot.

I have a TODO somewhere to retry failed fetches, but obviously I never
implemented it. The deadline parameter should really be passed into Fetch but
20s always seemed like a safe. I think having it "low" like this might have
occasionally failed on the subversion fs as well.

Anyway - happy to increase this, though it'd be nice get some metrics on how
long these fetches take. Like, 90% complete in under 5s, 10% under 10s, etc etc.

[Ken Rockot(use gerrit already), 2014-10-22 17:06:19]

On 2014/10/22 16:48:37, kalman wrote:
> > (though kinda
> > weird given that the caching is opaque to fs consumers).
> 
> Before I look at this CL: yep, it would be bad for any CacheMissError to leak
> out of the CachingFileSystem. CachingFileSystem should catch these and do what
> it wants with them, like turn them into FileSystemErrors I suppose.
> 
> Though that said, I will now look at the code to see if my comment makes
sense,
> because I wouldn't have thought CacheMissErrors would even need to leak out at
> all (i.e. that it was all captured in the implementation details of
> CachingFileSystem). It may also not make sense for other reasons.

Throwing a FileNotFound error directly actually seems like the right approach.
It
seems kind of weird to have it raise a CacheMissError and immediately re-raise
it
as a FileNotFoundError.

On 2014/10/22 16:51:58, kalman wrote:
> One more pre-review comment.
> 
>
https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
> File chrome/common/extensions/docs/server2/appengine_url_fetcher.py (right):
> 
>
https://codereview.chromium.org/660383002/diff/1/chrome/common/extensions/doc...
> chrome/common/extensions/docs/server2/appengine_url_fetcher.py:45:
deadline=40,
> On 2014/10/22 03:19:54, Ken Rockot wrote:
> > On 2014/10/20 21:06:57, kalman wrote:
> > > Do we know this is failing?
> > 
> > Yes. I was seeing timeouts occurring at the 20 second mark. I don't know how
> > consistent it is, but really I also don't see any harm in letting tasks
block
> > for a while on requests.
> > 
> > Alternatively we can turn this way down and let tasks fail when remote
> services
> > are unresponsive. This frees up the task queue for other stuff.
> > 
> > If the gitiles delays are really sporadic per request, that might make
sense.
> > Otherwise it would probably just cause the queue to churn a lot.
> 
> I have a TODO somewhere to retry failed fetches, but obviously I never
> implemented it. The deadline parameter should really be passed into Fetch but
> 20s always seemed like a safe. I think having it "low" like this might have
> occasionally failed on the subversion fs as well.
> 
> Anyway - happy to increase this, though it'd be nice get some metrics on how
> long these fetches take. Like, 90% complete in under 5s, 10% under 10s, etc
etc.

Metrics SGTM.

[not at google - send to devlin, 2014-10-22 18:08:48]

Ok, I'd say I made a light-to-medium depth review of this CL. As I mentioned
it's a little overwhelming for me, and finding a way to split it up a bit would
be very helpful.

https://codereview.chromium.org/660383002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/cache_chain_object_store.py (right):

https://codereview.chromium.org/660383002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/cache_chain_object_store.py:33: for
object_store in self._object_stores])
I don't think you need the [] here. All() can take a generator, so it can just
be All(object_store.SetMulti(mapping) for object_store in self.object_stores).

https://codereview.chromium.org/660383002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/file_system.py (right):

https://codereview.chromium.org/660383002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/file_system.py:163: return
self.GetStableIdentity()
(comment written before our chat, but I'll include it anyway)

If you did this rename in a pre-patch it'd make reviewing this patch easier
(fewer files changed), and mean that you don't need the GetIdentity() legacy
alias around.

You could even add that GetUnstableIdentity function which throws an
AssertionError for now, but just so that the comment exists to contrast with
GetStableIdentity.

https://codereview.chromium.org/660383002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/file_system.py:174: def
GetUnstableIdentity(self):
And yeah, it would be nice to think about these names and/or comments carefully.
I understand the difference of course (given our discussion yesterday) but I
don't imagine the names would help others.

May at least for now rewrite these comments and see how that might influence the
name?

As just discussed now, perhaps we should leave GetIdentity (but consider naming
it something less confusing) and add a GetVersion method, since only 2 things
care about it: CachingFileSystem and CompiledFileSystem. The equivalent, other
option would be to stick with the 2 different methods like you have, but now
we're back to the naming issue.

GetIdentity vs GetIdentityWithVersion?
GetLocation vs GetLocationWithVersion?

Maybe GetLocation would be a good (re)name for GetIdentity anyway.

https://codereview.chromium.org/660383002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/660383002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/handler.py:22: def __init__(self,
request):
Can you do this change in a separate patch?

https://codereview.chromium.org/660383002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/refresh_tracker.py (right):

https://codereview.chromium.org/660383002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/refresh_tracker.py:24: class
RefreshTracker(object):
Tests for this class?

[Ken Rockot(use gerrit already), 2014-10-23 00:59:11]

Patchset #3 (id:60001) has been deleted

[Ken Rockot(use gerrit already), 2014-10-23 00:59:18]

Patchset #3 (id:80001) has been deleted

[Ken Rockot(use gerrit already), 2014-10-23 00:59:26]

Patchset #3 (id:100001) has been deleted

[Ken Rockot(use gerrit already), 2014-10-23 00:59:32]

Patchset #3 (id:120001) has been deleted

[Ken Rockot(use gerrit already), 2014-10-23 00:59:39]

Patchset #3 (id:140001) has been deleted

[Ken Rockot(use gerrit already), 2014-10-23 19:34:23]

Patchset #3 (id:160001) has been deleted

[Ken Rockot(use gerrit already), 2014-10-23 19:50:19]

Patchset #4 (id:200001) has been deleted

[Ken Rockot(use gerrit already), 2014-10-23 19:50:25]

Patchset #3 (id:180001) has been deleted

[Ken Rockot(use gerrit already), 2014-10-23 19:54:24]

Patchset #3 (id:220001) has been deleted

[Ken Rockot(use gerrit already), 2014-10-23 20:01:18]

Patchset #3 (id:240001) has been deleted

[Ken Rockot(use gerrit already), 2014-10-23 22:29:57]

Patchset #3 (id:260001) has been deleted

[Ken Rockot(use gerrit already), 2014-10-23 22:30:03]

Patchset #3 (id:280001) has been deleted

[Ken Rockot(use gerrit already), 2014-10-23 22:36:16]

Please take another look

https://codereview.chromium.org/660383002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/cache_chain_object_store.py (right):

https://codereview.chromium.org/660383002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/cache_chain_object_store.py:33: for
object_store in self._object_stores])
On 2014/10/22 18:08:47, kalman wrote:
> I don't think you need the [] here. All() can take a generator, so it can just
> be All(object_store.SetMulti(mapping) for object_store in self.object_stores).

Done.

https://codereview.chromium.org/660383002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/660383002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/handler.py:22: def __init__(self,
request):
On 2014/10/22 18:08:47, kalman wrote:
> Can you do this change in a separate patch?

Done.

https://codereview.chromium.org/660383002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/refresh_tracker.py (right):

https://codereview.chromium.org/660383002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/refresh_tracker.py:24: class
RefreshTracker(object):
On 2014/10/22 18:08:48, kalman wrote:
> Tests for this class?

Done.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
File chrome/common/extensions/docs/server2/app_yaml_helper_test.py (right):

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/app_yaml_helper_test.py:95:
TestFileSystem(test_data, relative_to=SERVER2), updates[:int(commit)])
This is necessary because I changed mock FS to use string commit IDs

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
File chrome/common/extensions/docs/server2/caching_file_system.py (right):

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/caching_file_system.py:34:
optargs['start_empty'] = optargs.get('start_empty', not versioned)
This is gross. Is there a more clever way to express this kind of behavior? Also
we could just make start_empty a kw arg of create_object_store and nix optargs
altogether.

[not at google - send to devlin, 2014-10-24 00:04:52]

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
File chrome/common/extensions/docs/server2/caching_file_system.py (right):

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/caching_file_system.py:28: version =
file_system.GetVersion()
It seems like these changes should be reflected in CachingFileSystemTest

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/caching_file_system.py:29: versioned =
try_versioning and version is not None
And do you need to update CompiledFileSystem at all?

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/caching_file_system.py:34:
optargs['start_empty'] = optargs.get('start_empty', not versioned)
On 2014/10/23 22:36:16, Ken Rockot wrote:
> This is gross. Is there a more clever way to express this kind of behavior?
Also
> we could just make start_empty a kw arg of create_object_store and nix optargs
> altogether.

Maybe only have a single flag like "controls_versioning" which controls both
start_empty and versioned? They're mutually exclusive.

(the answer to your question, at face value, is no I don't think there's a more
clever way)

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
File chrome/common/extensions/docs/server2/commit_tracker.py (right):

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/commit_tracker.py:13: self._store =
object_store_creator.Create(str, category='commits',
s/str/CommitTracker/ ?
Also it wouldn't need a category then, but, I guess it can't hurt.
Also perhaps explain why you need (to override) the start_empty.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
File chrome/common/extensions/docs/server2/cron_servlet.py (right):

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/cron_servlet.py:123: tasks.append(('%s/%s'
% (name, path)).strip('/'))
nit: list comp? tasks += [('%s/%s' ...)]

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/cron_servlet.py:125:
refresh_tracker.RegisterRefresh(master_commit, tasks).Get()
Maybe this should actually be StartRefresh or something? And you should comment
that the other side of this is the RefreshTracker that gets constructed in
refresh_servlet.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
File chrome/common/extensions/docs/server2/gitiles_file_system.py (right):

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/gitiles_file_system.py:237: master, even
if they are pinned to a specific commit.'''
This seems more like a code comment than a docstring, so it should be # comment.

and nit: I prefer putting those inside the conditional that they apply to,
especially in a chain of if..elif..else.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/gitiles_file_system.py:244: GITILES_BASE,
GITILES_SRC_ROOT, GITILES_BRANCHES_PATH, self._branch)
These all share GITILES_BASE and GITILES_SRC_ROOT as their first arguments...
might be easier to just prepend them within that StringIdentity call?

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/gitiles_file_system.py:250: return None
return self._commit ?

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
File chrome/common/extensions/docs/server2/refresh_servlet.py (right):

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/refresh_servlet.py:100: data_source =
_InstanceMasterCommitUpdater(commit)
I don't see _InstanceMasterCommitUpdater anywhere?

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/refresh_servlet.py:114: return
What happens if source_path is None but there are no refresh paths? It falls
through, and... it crashes?

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/refresh_servlet.py:129:
refresh_tracker.MarkTaskComplete(commit, path).Get()
Use Then() not Get()?

You could even go so far as:

(refresh_tracker.MarkTaskComplete(commit, path)
 .Then(lambda: refresh_tracker.GetRefreshComplete(commit))
 .Then(lambda completed:
         commit_tracker.Set('master', commit) if completed else None)
 .Get())

which is pretty fun.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/refresh_servlet.py:130:
refresh_tracker.GetRefreshComplete(commit).Then(resolve).Get()
You should mention how this relates to cron servlet.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
File chrome/common/extensions/docs/server2/refresh_tracker.py (right):

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/refresh_tracker.py:30:
category='refresh_task_completions', start_empty=False)
It seems like you're double-namespacing these things?

_RefreshWorkOrder is just a set, so you only need to store sets in _work_orders.
The only advantage I could see in making it a class is to get it namespaced on
_RefreshWorkOrder, but, that's what category is for.

Likewise, isn't _RefreshTaskCompletion just an object?

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/refresh_tracker.py:49: return
len(future.Get()) == len(work_order.tasks)
Can you turn this .Get() into a .Then()?

[Ken Rockot(use gerrit already), 2014-10-24 21:26:54]

Thanks for the review. I think I've covered all your comments.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
File chrome/common/extensions/docs/server2/caching_file_system.py (right):

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/caching_file_system.py:28: version =
file_system.GetVersion()
On 2014/10/24 00:04:50, kalman wrote:
> It seems like these changes should be reflected in CachingFileSystemTest

Done. Added a test to verify caching behavior on versioned file systems.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/caching_file_system.py:29: versioned =
try_versioning and version is not None
On 2014/10/24 00:04:50, kalman wrote:
> And do you need to update CompiledFileSystem at all?

I don't think so? This change only affects the caching of versioned filesystems,
of which Gitiles is the only one. I haven't given much thought to how/if
CompiledFS could use it, but as long as CompiledFS remains unversioned its
behavior is not impacted by this change.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/caching_file_system.py:34:
optargs['start_empty'] = optargs.get('start_empty', not versioned)
On 2014/10/24 00:04:50, kalman wrote:
> On 2014/10/23 22:36:16, Ken Rockot wrote:
> > This is gross. Is there a more clever way to express this kind of behavior?
> Also
> > we could just make start_empty a kw arg of create_object_store and nix
optargs
> > altogether.
> 
> Maybe only have a single flag like "controls_versioning" which controls both
> start_empty and versioned? They're mutually exclusive.
> 
> (the answer to your question, at face value, is no I don't think there's a
more
> clever way)

OK - Leaving as-is then.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
File chrome/common/extensions/docs/server2/commit_tracker.py (right):

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/commit_tracker.py:13: self._store =
object_store_creator.Create(str, category='commits',
On 2014/10/24 00:04:50, kalman wrote:
> s/str/CommitTracker/ ?
> Also it wouldn't need a category then, but, I guess it can't hurt.
> Also perhaps explain why you need (to override) the start_empty.

Oh. Epiphany. I was making incorrect assumptions about the implications of the
class name v.s. category name. In retrospect those assumptions are nonsensical.
But I get it now, and it's much simpler than I expected. :D

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
File chrome/common/extensions/docs/server2/cron_servlet.py (right):

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/cron_servlet.py:123: tasks.append(('%s/%s'
% (name, path)).strip('/'))
On 2014/10/24 00:04:51, kalman wrote:
> nit: list comp? tasks += [('%s/%s' ...)]

Done.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/cron_servlet.py:125:
refresh_tracker.RegisterRefresh(master_commit, tasks).Get()
On 2014/10/24 00:04:51, kalman wrote:
> Maybe this should actually be StartRefresh or something? And you should
comment
> that the other side of this is the RefreshTracker that gets constructed in
> refresh_servlet.

Done.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
File chrome/common/extensions/docs/server2/gitiles_file_system.py (right):

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/gitiles_file_system.py:237: master, even
if they are pinned to a specific commit.'''
On 2014/10/24 00:04:51, kalman wrote:
> This seems more like a code comment than a docstring, so it should be #
comment.
> 
> and nit: I prefer putting those inside the conditional that they apply to,
> especially in a chain of if..elif..else.

Done.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/gitiles_file_system.py:244: GITILES_BASE,
GITILES_SRC_ROOT, GITILES_BRANCHES_PATH, self._branch)
On 2014/10/24 00:04:51, kalman wrote:
> These all share GITILES_BASE and GITILES_SRC_ROOT as their first arguments...
> might be easier to just prepend them within that StringIdentity call?

Done.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/gitiles_file_system.py:250: return None
On 2014/10/24 00:04:51, kalman wrote:
> return self._commit ?

Lol. Right. Done.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
File chrome/common/extensions/docs/server2/refresh_servlet.py (right):

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/refresh_servlet.py:100: data_source =
_InstanceMasterCommitUpdater(commit)
On 2014/10/24 00:04:51, kalman wrote:
> I don't see _InstanceMasterCommitUpdater anywhere?

Oops. Yeah this is old dead code. Removed.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/refresh_servlet.py:114: return
This works just fine (when source_path is None it means the top-level task is
the only one for that DataSource; e.g. /_refresh/api_list just does a refresh of
APIListDataSource with no path, where as the apis data source requires
subpaths).

It's confusing though and really only useful for testing (so I could do
something like /_refresh/apis to manually verify all API refreshes without doing
a full cron or individually running _refresh/apis/apps/usb,
_refresh/apis/extensions/tabs, etc).

I don't think it should stay, so I killed it.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/refresh_servlet.py:129:
refresh_tracker.MarkTaskComplete(commit, path).Get()
On 2014/10/24 00:04:51, kalman wrote:
> Use Then() not Get()?
> 
> You could even go so far as:
> 
> (refresh_tracker.MarkTaskComplete(commit, path)
>  .Then(lambda: refresh_tracker.GetRefreshComplete(commit))
>  .Then(lambda completed:
>          commit_tracker.Set('master', commit) if completed else None)
>  .Get())
> 
> which is pretty fun.

Neat. I'm all for chaining futures of you're down with that. Some folks are
queasy about anonymous functions. :D

Done.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/refresh_servlet.py:130:
refresh_tracker.GetRefreshComplete(commit).Then(resolve).Get()
On 2014/10/24 00:04:52, kalman wrote:
> You should mention how this relates to cron servlet.

Done.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
File chrome/common/extensions/docs/server2/refresh_tracker.py (right):

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/refresh_tracker.py:30:
category='refresh_task_completions', start_empty=False)
On 2014/10/24 00:04:52, kalman wrote:
> It seems like you're double-namespacing these things?
> 
> _RefreshWorkOrder is just a set, so you only need to store sets in
_work_orders.
> The only advantage I could see in making it a class is to get it namespaced on
> _RefreshWorkOrder, but, that's what category is for.
> 
> Likewise, isn't _RefreshTaskCompletion just an object?

As noted on CommitTracker, yeah, I was using this incorrectly. Fixed.

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/refresh_tracker.py:49: return
len(future.Get()) == len(work_order.tasks)
On 2014/10/24 00:04:52, kalman wrote:
> Can you turn this .Get() into a .Then()?

Done.

[not at google - send to devlin, 2014-10-24 23:36:07]

lgtm

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
File chrome/common/extensions/docs/server2/refresh_servlet.py (right):

https://codereview.chromium.org/660383002/diff/300001/chrome/common/extension...
chrome/common/extensions/docs/server2/refresh_servlet.py:129:
refresh_tracker.MarkTaskComplete(commit, path).Get()
On 2014/10/24 21:26:54, Ken Rockot wrote:
> On 2014/10/24 00:04:51, kalman wrote:
> > Use Then() not Get()?
> > 
> > You could even go so far as:
> > 
> > (refresh_tracker.MarkTaskComplete(commit, path)
> >  .Then(lambda: refresh_tracker.GetRefreshComplete(commit))
> >  .Then(lambda completed:
> >          commit_tracker.Set('master', commit) if completed else None)
> >  .Get())
> > 
> > which is pretty fun.
> 
> Neat. I'm all for chaining futures of you're down with that. Some folks are
> queasy about anonymous functions. :D
> 
> Done.

I think the Python creators are against anonymous functions.

https://codereview.chromium.org/660383002/diff/340001/chrome/common/extension...
File chrome/common/extensions/docs/server2/gitiles_file_system.py (right):

https://codereview.chromium.org/660383002/diff/340001/chrome/common/extension...
chrome/common/extensions/docs/server2/gitiles_file_system.py:243: return
('%s/%s@' % (GITILES_BASE, GITILES_SRC_ROOT)).join(
why does the string end with an '@'?

[Ken Rockot(use gerrit already), 2014-10-24 23:43:18]

https://codereview.chromium.org/660383002/diff/340001/chrome/common/extension...
File chrome/common/extensions/docs/server2/gitiles_file_system.py (right):

https://codereview.chromium.org/660383002/diff/340001/chrome/common/extension...
chrome/common/extensions/docs/server2/gitiles_file_system.py:243: return
('%s/%s@' % (GITILES_BASE, GITILES_SRC_ROOT)).join(
On 2014/10/24 23:36:07, kalman wrote:
> why does the string end with an '@'?

I... don't really know. I crossed some wires when rewriting this.

Fixed.

[Ken Rockot(use gerrit already), 2014-10-24 23:50:01]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-10-24 23:51:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/660383002/360001

[commit-bot: I haz the power, 2014-10-24 23:55:09]

Committed patchset #6 (id:360001)

[commit-bot: I haz the power, 2014-10-24 23:55:59]

Patchset 6 (id:??) landed as
https://crrev.com/360062747ecb8079f2c7467bc5c2fef914e1beb9
Cr-Commit-Position: refs/heads/master@{#301236}