Store each file as "hash/<sha1>", list of hashes as "meta/@<rev>" and serve them at "serve_file/@<r…

Store each file as "hash/<sha1>", list of hashes as "meta/@<rev>" and serve them at "serve_file/@<rev>/<file-path>".

BUG=459167
R=mnaganov@chromium.org

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

[dgozman, 2015-02-24 09:55:11]

dgozman@chromium.org changed reviewers:
+ mnaganov@chromium.org, pfeldman@chromium.org

[dgozman, 2015-02-24 09:55:11]

Could you please take a look?

[mnaganov (inactive), 2015-02-24 14:40:32]

The idea looks good, but there are some issues with the implementation.

Do you plan to abandon using appcache and manifest files? If not, there should
be something for serving manifest files from serve_file/@rev/xxx.manifest URLs.

https://codereview.chromium.org/954693002/diff/1/gae/main.py
File gae/main.py (right):

https://codereview.chromium.org/954693002/diff/1/gae/main.py#newcode26
gae/main.py:26: def get_file_hash(self, meta_content, path):
We should probably cache '(tag, path) -> hash' somewhere in memory to avoid
parsing the meta file each time as well as looking up file hashes in linear
time.

https://codereview.chromium.org/954693002/diff/1/gae/main.py#newcode38
gae/main.py:38: if not content and tag_type == 'file':
No, we shouldn't cache these resources under 'cache_key_name' at all. The file
reader already caches them using GCE file paths. The reason why we use
'cache_key_name' is because the contents are extracted on the fly from zip
archives. But for resources that we just read directly from GCE this will end up
creating two copies of each resource in the cache.

You should branch on 'tag_type == file' earlier, and just not access cache in
this branch -- file_reader will do caching for you.

https://codereview.chromium.org/954693002/diff/1/gce/uploader_iteration.sh
File gce/uploader_iteration.sh (right):

https://codereview.chromium.org/954693002/diff/1/gce/uploader_iteration.sh#ne...
gce/uploader_iteration.sh:131: find -type f -not -name $MANIFEST_FILE_NAME
-print | sed -e 's#^\./##' >> $MANIFEST_FILE_NAME
Perhaps, we can store this list in a variable and then reuse it in the new code
to avoid duplication?

https://codereview.chromium.org/954693002/diff/1/gce/uploader_iteration.sh#ne...
gce/uploader_iteration.sh:134: META_FILE_NAME="$META_DIR/@$REVISION"
I suggest to move this block after creating the zip file (but before popd).
Otherwise, this creates a false impression that those files also end up in the
archive.

[dgozman, 2015-02-25 11:37:14]

Added manifest and more caching. PTAL

https://codereview.chromium.org/954693002/diff/1/gae/main.py
File gae/main.py (right):

https://codereview.chromium.org/954693002/diff/1/gae/main.py#newcode26
gae/main.py:26: def get_file_hash(self, meta_content, path):
On 2015/02/24 14:40:32, mnaganov (cr) wrote:
> We should probably cache '(tag, path) -> hash' somewhere in memory to avoid
> parsing the meta file each time as well as looking up file hashes in linear
> time.

Done.

https://codereview.chromium.org/954693002/diff/1/gae/main.py#newcode38
gae/main.py:38: if not content and tag_type == 'file':
On 2015/02/24 14:40:32, mnaganov (cr) wrote:
> No, we shouldn't cache these resources under 'cache_key_name' at all. The file
> reader already caches them using GCE file paths. The reason why we use
> 'cache_key_name' is because the contents are extracted on the fly from zip
> archives. But for resources that we just read directly from GCE this will end
up
> creating two copies of each resource in the cache.
> 
> You should branch on 'tag_type == file' earlier, and just not access cache in
> this branch -- file_reader will do caching for you.

Done.

https://codereview.chromium.org/954693002/diff/1/gce/uploader_iteration.sh
File gce/uploader_iteration.sh (right):

https://codereview.chromium.org/954693002/diff/1/gce/uploader_iteration.sh#ne...
gce/uploader_iteration.sh:131: find -type f -not -name $MANIFEST_FILE_NAME
-print | sed -e 's#^\./##' >> $MANIFEST_FILE_NAME
On 2015/02/24 14:40:32, mnaganov (cr) wrote:
> Perhaps, we can store this list in a variable and then reuse it in the new
code
> to avoid duplication?

I've added manifest file there, so it's different now.

https://codereview.chromium.org/954693002/diff/1/gce/uploader_iteration.sh#ne...
gce/uploader_iteration.sh:134: META_FILE_NAME="$META_DIR/@$REVISION"
On 2015/02/24 14:40:32, mnaganov (cr) wrote:
> I suggest to move this block after creating the zip file (but before popd).
> Otherwise, this creates a false impression that those files also end up in the
> archive.

Done.

[mnaganov (inactive), 2015-02-25 12:56:35]

Thanks! Some minor comments.

https://codereview.chromium.org/954693002/diff/20001/gae/main.py
File gae/main.py (right):

https://codereview.chromium.org/954693002/diff/20001/gae/main.py#newcode36
gae/main.py:36: self.abort(404)
Let's just return null here to make it clear that this function is not a
top-level request handler.

https://codereview.chromium.org/954693002/diff/20001/gae/main.py#newcode38
gae/main.py:38: for line in meta_content.split('\n'):
In case when the path isn't listed in the meta file, we'll be reparsing it again
and again. I guess, we can put a cache entry indicating that the particular meta
file has already been parsed.

https://codereview.chromium.org/954693002/diff/20001/gae/main.py#newcode39
gae/main.py:39: index = line.find(':')
You just do another split here, so you can just write:

path_and_hash = line.split(':')
if len(path_and_hash) == 2:
  ... use path_and_hash[0] and path_and_hash[1]

[dgozman, 2015-02-25 14:22:09]

PTAL

https://codereview.chromium.org/954693002/diff/20001/gae/main.py
File gae/main.py (right):

https://codereview.chromium.org/954693002/diff/20001/gae/main.py#newcode36
gae/main.py:36: self.abort(404)
On 2015/02/25 12:56:35, mnaganov (cr) wrote:
> Let's just return null here to make it clear that this function is not a
> top-level request handler.

Nice catch! Done.

https://codereview.chromium.org/954693002/diff/20001/gae/main.py#newcode38
gae/main.py:38: for line in meta_content.split('\n'):
On 2015/02/25 12:56:35, mnaganov (cr) wrote:
> In case when the path isn't listed in the meta file, we'll be reparsing it
again
> and again. I guess, we can put a cache entry indicating that the particular
meta
> file has already been parsed.

Done.

https://codereview.chromium.org/954693002/diff/20001/gae/main.py#newcode39
gae/main.py:39: index = line.find(':')
On 2015/02/25 12:56:35, mnaganov (cr) wrote:
> You just do another split here, so you can just write:
> 
> path_and_hash = line.split(':')
> if len(path_and_hash) == 2:
>   ... use path_and_hash[0] and path_and_hash[1]

Done.

[mnaganov (inactive), 2015-02-25 15:33:12]

LGTM, but please let me test this first before committing. I'll let you know
when it's OK to land.

[mnaganov (inactive), 2015-02-25 17:34:45]

Looks like it works :) Please submit.

[dgozman, 2015-02-26 10:38:45]

Committed patchset #4 (id:60001) manually as 294235 (presubmit successful).

[dgozman, 2015-02-26 10:39:18]

On 2015/02/26 10:38:45, dgozman wrote:
> Committed patchset #4 (id:60001) manually as 294235 (presubmit successful).

Could you please deploy the changes?

[mnaganov (inactive), 2015-02-26 12:50:53]

On 2015/02/26 10:39:18, dgozman wrote:
> On 2015/02/26 10:38:45, dgozman wrote:
> > Committed patchset #4 (id:60001) manually as 294235 (presubmit successful).
> 
> Could you please deploy the changes?

Done. 190912 was the first revision uploaded using the new GCE script. You can
now try
https://chrome-devtools-frontend.appspot.com/serve_file/@190912/inspector.html