Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
453713002:
Docserver: Generate a table of extension/app API owners (Closed)
|
Created:
6 years, 4 months ago by ahernandez Modified:
5 years, 6 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionDocserver: Generate a table of extension/app API owners
BUG=400760
NOTRY=True
Committed: https://crrev.com/473591b50d408fa2dda99f9b11d326db60a430a1
Cr-Commit-Position: refs/heads/master@{#292223}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : #
Total comments: 10
Patch Set 3 : Simpler logic #
Total comments: 2
Patch Set 4 : #
Total comments: 9
Patch Set 5 : #
Total comments: 17
Patch Set 6 : mlg rebasing #Messages
Total messages: 45 (0 generated)
This is by no means ready for review. This is more of an FYI, and I'd like to get some high level comments (e.g. is this the right direction? How should owners.html actually look? etc). Also, what do you think of the notes section? It doesn't look like it will work out in practice since certain comments are specific to a listed owner.
On 2014/08/08 00:45:04, ahernandez wrote: > This is by no means ready for review. This is more of an FYI, and I'd like to > get some high level comments (e.g. is this the right direction? How should > owners.html actually look? etc). > > Also, what do you think of the notes section? It doesn't look like it will work > out in practice since certain comments are specific to a listed owner. I forgot to mention, this should be ready to be patched in and viewed from preview.py.
awesome x100 https://codereview.chromium.org/453713002/diff/20001/chrome/common/extensions... File chrome/common/extensions/docs/server2/owners_data_source.py (right): https://codereview.chromium.org/453713002/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/owners_data_source.py:1: import posixpath copyright https://codereview.chromium.org/453713002/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/owners_data_source.py:45: if _OWNERS in files: we should only need to check 1 deep, not Walk over the whole thing - the nice part about that will be warning for APIs that don't have owners (and by "warning" I mean listing them in the table with a red <no owner> marker) https://codereview.chromium.org/453713002/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/owners_data_source.py:55: return api_owners we need to future-ise this. https://codereview.chromium.org/453713002/diff/20001/chrome/common/extensions... File chrome/common/extensions/docs/templates/public/owners.html (right): https://codereview.chromium.org/453713002/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/templates/public/owners.html:1: <html> hey let's give markdown a try here, see if it works? that'll be much easier to add ad-hoc nodes to. i mentioned something silly in the bug about a JSON file declaring more stuff; that's silly, we can just write free-form text, but that'd be easier if it were markdown vs html. https://codereview.chromium.org/453713002/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/templates/public/owners.html:3: <link href="{{static}}/css/out/site.css" rel="stylesheet" type="text/css"> this style looks pretty, but wow it's extremely spacious and looks kinda silly.
https://codereview.chromium.org/453713002/diff/20001/chrome/common/extensions... File chrome/common/extensions/docs/server2/owners_data_source.py (right): https://codereview.chromium.org/453713002/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/owners_data_source.py:48: api_owners.append({ ooh another idea - TODO for now because I don't know how to do this yet - but we should also mark any owners that aren't on the project anymore (I'm seeing ikianerator in that table). I'm not sure if we can get that data externally though.
https://codereview.chromium.org/453713002/diff/20001/chrome/common/extensions... File chrome/common/extensions/docs/templates/public/owners.html (right): https://codereview.chromium.org/453713002/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/templates/public/owners.html:1: <html> On 2014/08/08 14:42:27, kalman wrote: > hey let's give markdown a try here, see if it works? that'll be much easier to > add ad-hoc nodes to. > > i mentioned something silly in the bug about a JSON file declaring more stuff; > that's silly, we can just write free-form text, but that'd be easier if it were > markdown vs html. Not sure if markdown will work. I tried this: API | Owners | Notes -----------------|------------------|--------------- {{#entry:owners.apis}} {{entry.apiName}}|{{{entry.owners}}}|{{entry.notes}} {{/owners.apis}} And it doesn't generate the table properly because the markdown has to be compiled before handlebar processes it.
https://codereview.chromium.org/453713002/diff/20001/chrome/common/extensions... File chrome/common/extensions/docs/templates/public/owners.html (right): https://codereview.chromium.org/453713002/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/templates/public/owners.html:1: <html> On 2014/08/08 20:46:07, ahernandez wrote: > On 2014/08/08 14:42:27, kalman wrote: > > hey let's give markdown a try here, see if it works? that'll be much easier to > > add ad-hoc nodes to. > > > > i mentioned something silly in the bug about a JSON file declaring more stuff; > > that's silly, we can just write free-form text, but that'd be easier if it > were > > markdown vs html. > > Not sure if markdown will work. I tried this: > > API | Owners | Notes > -----------------|------------------|--------------- > {{#entry:owners.apis}} > {{entry.apiName}}|{{{entry.owners}}}|{{entry.notes}} > {{/owners.apis}} > > And it doesn't generate the table properly because the markdown has to be > compiled before handlebar processes it. ah, bummer. oh well.
Shouldn't calling this: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... with skip_not_found=True mean that it will never raise a FileNotFoundError?
On 2014/08/08 22:20:25, ahernandez wrote: > Shouldn't calling this: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > with skip_not_found=True mean that it will never raise a FileNotFoundError? looks like it?
On 2014/08/08 22:59:43, kalman wrote: > On 2014/08/08 22:20:25, ahernandez wrote: > > Shouldn't calling this: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > > > with skip_not_found=True mean that it will never raise a FileNotFoundError? > > looks like it? I just wanted to make sure. It's not behaving as expected, but I imagine it won't be a difficult fix.
Here's a more polished patch, PTAL. https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... File chrome/common/extensions/docs/server2/cron_servlet_test.py (right): https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/cron_servlet_test.py:90: read_count=0, I'm not sure what to do about this test. One of the file systems has a bunch of read counts because of OwnersDataSource. I'm passing all other tests besides this.
https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... File chrome/common/extensions/docs/server2/cron_servlet_test.py (right): https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/cron_servlet_test.py:90: read_count=0, On 2014/08/13 23:24:28, ahernandez wrote: > I'm not sure what to do about this test. One of the file systems has a bunch of > read counts because of OwnersDataSource. I'm passing all other tests besides > this. This test is supposed to catch anything that doesn't cache the file system data in its Cron run. At least, with respect to the next Cron run. What's weird is that I'm staring at it and OwnersDataSource looks fine from that perspective. https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... File chrome/common/extensions/docs/server2/owners_data_source.py (right): https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/owners_data_source.py:18: _OWNER_BASE_URL = '<a href=' + CODEREVIEW_SERVER + '/user/%s>%s</a>' Don't generate HTML here, generate enough data in the model to be able to generate it in the template. Maybe you'll need a noOwner flag and to pass CODEREVIEW_SERVER up to the templates, etc. https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/owners_data_source.py:38: return ', '.join([_OWNER_BASE_URL % (owner, owner[:owner.find('@')]) You should be able to join() a generator (i.e. no need to surround in []). https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/owners_data_source.py:56: for file_ in self._host_fs.ReadSingle(root).Get(): Then() https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/owners_data_source.py:61: skip_not_found=True).Get() Then() https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/owners_data_source.py:70: posixpath.join(BROWSER_CHROME_EXTENSIONS, _OWNERS)).Get() Then() And splitting up these entirely serial methods into async blocks can be challenging. To simplify the logic here perhaps add a depth argument to FileSystem.Walk. Set it to 1 for this method. Walk returns things like lists of files and that's quite a lot more convenient than doing it by hand.
https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... File chrome/common/extensions/docs/server2/owners_data_source.py (right): https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/owners_data_source.py:45: self._cache = server_instance.object_store_creator.Create(OwnersDataSource) The reason this is failing the cron servlet is that on the second cron run, this cache here is recreated. Which means that the collect code below has to be run all over again because the cache is empty, which results in several file system reads. I'm not sure how to get around that. Is there a way to save the cache between cron runs?
https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... File chrome/common/extensions/docs/server2/owners_data_source.py (right): https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/owners_data_source.py:45: self._cache = server_instance.object_store_creator.Create(OwnersDataSource) On 2014/08/14 21:48:21, ahernandez wrote: > The reason this is failing the cron servlet is that on the second cron run, this > cache here is recreated. Which means that the collect code below has to be run > all over again because the cache is empty, which results in several file system > reads. I'm not sure how to get around that. Is there a way to save the cache > between cron runs? Hm, those object stores are supposed to stay around between tests, at least this comment makes me believe so: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... but I don't think that's the issue, the test says that there should be the same number of Stats but no re-reading because they shouldn't have changed. Might be missing something. How about let's try this with the Walk functionality, since that's an improvement anyway, and take it from there?
https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... File chrome/common/extensions/docs/server2/owners_data_source.py (right): https://codereview.chromium.org/453713002/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/owners_data_source.py:45: self._cache = server_instance.object_store_creator.Create(OwnersDataSource) On 2014/08/14 22:02:10, kalman wrote: > On 2014/08/14 21:48:21, ahernandez wrote: > > The reason this is failing the cron servlet is that on the second cron run, > this > > cache here is recreated. Which means that the collect code below has to be run > > all over again because the cache is empty, which results in several file > system > > reads. I'm not sure how to get around that. Is there a way to save the cache > > between cron runs? > > Hm, those object stores are supposed to stay around between tests, at least this > comment makes me believe so: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > but I don't think that's the issue, the test says that there should be the same > number of Stats but no re-reading because they shouldn't have changed. Might be > missing something. > > How about let's try this with the Walk functionality, since that's an > improvement anyway, and take it from there? I already have it implemented with Walk (I haven't uploaded another patchset because I can't fix the cron_servlet_test). I have used logging statements to verify that the caches indeed get reset, in both OwnersDataSource and APIDataSource. However, I can't figure out why that's happening. Hopefully I haven't been barking up the wrong tree. Should I upload the newest revision so you can have a look at that?
> I already have it implemented with Walk (I haven't uploaded another patchset > because I can't fix the cron_servlet_test). I have used logging statements to > verify that the caches indeed get reset, in both OwnersDataSource and > APIDataSource. However, I can't figure out why that's happening. Hopefully I > haven't been barking up the wrong tree. Should I upload the newest revision so > you can have a look at that? Yep I can have a look.
Here's the next patch set, PTAL. https://codereview.chromium.org/453713002/diff/100001/chrome/common/extension... File chrome/common/extensions/docs/server2/owners_data_source.py (right): https://codereview.chromium.org/453713002/diff/100001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source.py:56: return self._host_fs.Read((owners_file,), skip_not_found=True).Then( This call is what gets run during the second cron run because of the cache miss and increments the file system read count.
https://codereview.chromium.org/453713002/diff/100001/chrome/common/extension... File chrome/common/extensions/docs/server2/owners_data_source.py (right): https://codereview.chromium.org/453713002/diff/100001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source.py:56: return self._host_fs.Read((owners_file,), skip_not_found=True).Then( On 2014/08/14 22:17:18, ahernandez wrote: > This call is what gets run during the second cron run because of the cache miss > and increments the file system read count. I wonder if it has something to do with the skip_not_found? because the caching of the file system is supposed to be handling this. If you Read() a file in the Cron - that's all - then it should be cached for the next time around. I had thought. This can hardly be the first instance of calling a raw Read from a cron. Could you use a compiled fs here now anyway, since you're effectively compiling OWNERS files? Avoid the problem...
Using a compiled file system was an excellent suggestion. PTAL.
Growing. https://codereview.chromium.org/453713002/diff/120001/chrome/common/extension... File chrome/common/extensions/docs/server2/file_system.py (right): https://codereview.chromium.org/453713002/diff/120001/chrome/common/extension... chrome/common/extensions/docs/server2/file_system.py:154: |base| respectively. If |depth| is specified and greater than 0, Walk will Quick test for specifying depth=0, 1, or 2 plz. https://codereview.chromium.org/453713002/diff/120001/chrome/common/extension... File chrome/common/extensions/docs/server2/local_file_system.py (right): https://codereview.chromium.org/453713002/diff/120001/chrome/common/extension... chrome/common/extensions/docs/server2/local_file_system.py:96: raise Actually this should return a Future which raises a FileNotFoundError, not raise immediately. Look for places where exc_info is specified, I forget the Python call for this. https://codereview.chromium.org/453713002/diff/120001/chrome/common/extension... File chrome/common/extensions/docs/server2/owners_data_source.py (right): https://codereview.chromium.org/453713002/diff/120001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source.py:25: return [], 'See core extensions owners.' Let's share a string with 'Core Extensions/Apps Owners'. Like, 'Use one of the ' + CORE_OWNERS for example. Speaking of which - could you add a TODO for randomizing that list of core OWNERS? We have a persistent problem where people at the top of the list get more reviews. It'd be cool if they were randomised. Not really a priority, just a neat idea :) https://codereview.chromium.org/453713002/diff/120001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source.py:47: def _CreateAPIEntry(self, path, content): content is optional, so content=None. https://codereview.chromium.org/453713002/diff/120001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source.py:69: owners_file = posixpath.join(root, base, dir_, _OWNERS) For all of these posixpath calls, prefer using the functions in path_util (Join, etc). Mostly because they do path validation, but they're also more concise. There might be an equivalent for basename? https://codereview.chromium.org/453713002/diff/120001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source.py:70: if self._owners_fs.FileExists(owners_file).Get(): What happened to using .Then() everywhere? I'm wondering if to simplify all of this the file system methods (both on CompiledFileSystem and FileSystem) should respect a skip_not_found flag. Already ~half of the methods do. It would be so nice to be able to avoid calling Exists() and/or deal with exceptions. Actually this wouldn't be so hard. - Make CompiledFileSystem.ReadFrom* take a skip_not_found flag. - Same for CacheChainFileSystem. - Make FileSystem.ReadSingle take a skip_not_found flag, easy, just passes through to Read. Then you'd be able to write this so much more simply *and* it would be able to Then-ify. And we can likely simplify... other code... as we come across it. https://codereview.chromium.org/453713002/diff/120001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source.py:76: entry = self._owners_fs.GetFromFile(owners_file).Get() .Get() here as well. https://codereview.chromium.org/453713002/diff/120001/chrome/common/extension... File chrome/common/extensions/docs/templates/public/owners.html (right): https://codereview.chromium.org/453713002/diff/120001/chrome/common/extension... chrome/common/extensions/docs/templates/public/owners.html:42: <td>{{entry.notes}}</td> It'd be great if this could preserve the line breaks of the original OWNERS files. For that: (1) Give this a CSS rule with 'white-space: pre-line'. (2) Put this variable on its own line (or templates will inline it). (3) Modify owners_data_source.py to not strip '\n' characters, or put them back in, whatever.
I presume you're concentrating on getting gitiles up and running, and will return to this CL after?
On 2014/08/20 00:56:44, kalman wrote: > I presume you're concentrating on getting gitiles up and running, and will > return to this CL after? That is correct.
On 2014/08/20 01:01:55, ahernandez wrote: > On 2014/08/20 00:56:44, kalman wrote: > > I presume you're concentrating on getting gitiles up and running, and will > > return to this CL after? > > That is correct. I'm still concentrating on gitiles, but I wanted to mention that this patch is failing the cron_servlet_test again, even with the compiledFS. This makes me think again that there are caching issues going on.
On 2014/08/20 22:10:55, ahernandez wrote: > On 2014/08/20 01:01:55, ahernandez wrote: > > On 2014/08/20 00:56:44, kalman wrote: > > > I presume you're concentrating on getting gitiles up and running, and will > > > return to this CL after? > > > > That is correct. > > I'm still concentrating on gitiles, but I wanted to mention that this patch is > failing the cron_servlet_test again, even with the compiledFS. This makes me > think again that there are caching issues going on. Ok, bummer. Thanks for letting me know.
Compiled FS is still an improvement I presume. We can look into it from there.
Addressed comments, PTAL. https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... File chrome/common/extensions/docs/server2/caching_file_system.py (right): https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/server2/caching_file_system.py:63: return Future(value=dir_stat).Then(make_stat_info) I had to change it to this to avoid FileNotFoundErrors from being prematurely raised.
Did you say you fixed the caching issues? https://codereview.chromium.org/453713002/diff/120001/chrome/common/extension... File chrome/common/extensions/docs/server2/owners_data_source.py (right): https://codereview.chromium.org/453713002/diff/120001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source.py:47: def _CreateAPIEntry(self, path, content): On 2014/08/15 15:45:51, kalman wrote: > content is optional, so content=None. Oh, my bad. sorry. https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... File chrome/common/extensions/docs/server2/caching_file_system.py (right): https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/server2/caching_file_system.py:63: return Future(value=dir_stat).Then(make_stat_info) On 2014/08/21 18:07:27, ahernandez wrote: > I had to change it to this to avoid FileNotFoundErrors from being prematurely > raised. Heh, I needed to make that fix as well: https://codereview.chromium.org/429723005/diff/100001/chrome/common/extension... (I haven't had time to finish that patch obviously) Anyhow, I slightly prefer using a callback rather than .Then(). https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... File chrome/common/extensions/docs/server2/chained_compiled_file_system.py (right): https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/server2/chained_compiled_file_system.py:52: lambda compiled_fs: compiled_fs.GetFromFile(path, skip_not_found), skip_not_found=skip_not_found (sorry) https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... File chrome/common/extensions/docs/server2/compiled_file_system.py (right): https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/server2/compiled_file_system.py:213: return self._file_system.ReadSingle(path, skip_not_found).Then(next) skip_not_found=skip_not_found https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... File chrome/common/extensions/docs/server2/cron_servlet_test.py (right): https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/server2/cron_servlet_test.py:67: ConfigureFakeFetchers() Hm, why do you need this? I've tried very hard to not use ConfigureFakeFetchers anywhere, it basically implies a bad test setup. You can see how awful the fake fetchers are. https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... File chrome/common/extensions/docs/server2/file_system_test.py (right): https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/server2/file_system_test.py:69: self.assertEqual(all_files, []) For all of these, the expectation should go first. So this should be self.assertEqual([], all_dirs) and so on. https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... File chrome/common/extensions/docs/server2/owners_data_source.py (right): https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source.py:21: def _ParseOwnersFile(content, randomize): This method should probably be non-private with a comment "public for testing". https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source.py:41: random.shuffle(owners) Aww awesome :) https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source.py:71: return api_owners Nit: newline after this. The method is fairly long and the block of text is hard to follow. https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source.py:72: api_owners = [] Comment before |api_owners|: Get API owners from every OWNERS file that exists. https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source.py:77: api_owners.append(self._owners_fs.GetFromFile(owners_file, True)) skip_not_found=True https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source.py:79: def fix_core_owners(entry): Nit: declare this underneath the "Add an entry..." comment. Follow-up nit: newline after it. https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... File chrome/common/extensions/docs/server2/owners_data_source_test.py (right): https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source_test.py:65: self._owners_ds= OwnersDataSource(server_instance, ' ' before the '='. https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source_test.py:76: owners, notes = _ParseOwnersFile(owners_content, False) Can you use a named parameter for False here (randomize=False)? Passing booleans around without being named is hard to read. https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... File chrome/common/extensions/docs/templates/public/owners.html (right): https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/templates/public/owners.html:34: <td>{{entry.apiName}}</td> Your ID change below gave me an idea - let's give these rows IDs so that people can link to them (owners#storage). You can mostly use the API's name, but there's the problem of the "Core Extensions/Apps Owners" which looks silly as an ID. So you might need to add an ID field to the template. https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/templates/public/owners.html:42: <span class="warning">No owners.</span> And then you can link to the #core owners from here.
On 2014/08/21 21:39:58, kalman wrote: > Did you say you fixed the caching issues? I fixed the caching issues related to GitilesFS, but I'm stilling failing that test here. It was working before though so I wonder if it's related to changes made adding GitilesFS to the docserver.
On 2014/08/22 17:15:34, ahernandez wrote: > On 2014/08/21 21:39:58, kalman wrote: > > Did you say you fixed the caching issues? > > I fixed the caching issues related to GitilesFS, but I'm stilling failing that > test here. It was working before though so I wonder if it's related to changes > made adding GitilesFS to the docserver. Also, I just noticed there are a few duplicate entries for APIs that are under chrome/browser/extensions/api and extensions/browser/api. Should the duplicate entries just be removed? Or should there be a way of distinguishing from which directory tree the API comes from?
Regarding the caching issue, I'm very confused as to why it's not working. All the stats remain the same, and the keys used to access the memcache are also the same between cron runs. But I'm still getting cache misses and the reads are happening during the second cron run. Did you get a chance to look into this?
Looking now. https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... File chrome/common/extensions/docs/server2/owners_data_source.py (right): https://codereview.chromium.org/453713002/diff/160001/chrome/common/extension... chrome/common/extensions/docs/server2/owners_data_source.py:78: Found trailing spaces here as well.
(made this comment on the wrong CL. try again) Oh I think I know what it is. I haven't looked into exactly what's going on, but I bet it's due to the skip-not-found behaviour, where we're not caching it properly and it's falling through to multiple reads. Once that's sorted out we should add tests for it.
On 2014/08/26 01:23:50, kalman wrote: > (made this comment on the wrong CL. try again) > > Oh I think I know what it is. I haven't looked into exactly what's going on, but > I bet it's due to the skip-not-found behaviour, where we're not caching it > properly and it's falling through to multiple reads. > > Once that's sorted out we should add tests for it. I do believe you're right: the number of not founds is equal to the number of extra reads in cron_servlet_test.
On 2014/08/26 01:29:24, ahernandez wrote: > On 2014/08/26 01:23:50, kalman wrote: > > (made this comment on the wrong CL. try again) > > > > Oh I think I know what it is. I haven't looked into exactly what's going on, > but > > I bet it's due to the skip-not-found behaviour, where we're not caching it > > properly and it's falling through to multiple reads. > > > > Once that's sorted out we should add tests for it. > > I do believe you're right: the number of not founds is equal to the number of > extra reads in cron_servlet_test. Cool. I'll leave it to you. I presume it's an existing bug that was never caught.
On 2014/08/26 01:37:30, kalman wrote: > On 2014/08/26 01:29:24, ahernandez wrote: > > On 2014/08/26 01:23:50, kalman wrote: > > > (made this comment on the wrong CL. try again) > > > > > > Oh I think I know what it is. I haven't looked into exactly what's going on, > > but > > > I bet it's due to the skip-not-found behaviour, where we're not caching it > > > properly and it's falling through to multiple reads. > > > > > > Once that's sorted out we should add tests for it. > > > > I do believe you're right: the number of not founds is equal to the number of > > extra reads in cron_servlet_test. > > Cool. I'll leave it to you. I presume it's an existing bug that was never > caught. I'm pretty sure it's my fault, I introduced more skip_not_found functionality in this patch. Thanks for shedding light on the issue.
On 2014/08/26 01:41:51, ahernandez wrote: > On 2014/08/26 01:37:30, kalman wrote: > > On 2014/08/26 01:29:24, ahernandez wrote: > > > On 2014/08/26 01:23:50, kalman wrote: > > > > (made this comment on the wrong CL. try again) > > > > > > > > Oh I think I know what it is. I haven't looked into exactly what's going > on, > > > but > > > > I bet it's due to the skip-not-found behaviour, where we're not caching it > > > > properly and it's falling through to multiple reads. > > > > > > > > Once that's sorted out we should add tests for it. > > > > > > I do believe you're right: the number of not founds is equal to the number > of > > > extra reads in cron_servlet_test. > > > > Cool. I'll leave it to you. I presume it's an existing bug that was never > > caught. > > I'm pretty sure it's my fault, I introduced more skip_not_found functionality in > this patch. Thanks for shedding light on the issue. skip_not_found already exists: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... I guess the tests don't actually check caching behaviour: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... That's a nice patch there. Write a test to demonstrate the caching issue, fix it, done.
On 2014/08/26 02:28:34, kalman wrote: > On 2014/08/26 01:41:51, ahernandez wrote: > > On 2014/08/26 01:37:30, kalman wrote: > > > On 2014/08/26 01:29:24, ahernandez wrote: > > > > On 2014/08/26 01:23:50, kalman wrote: > > > > > (made this comment on the wrong CL. try again) > > > > > > > > > > Oh I think I know what it is. I haven't looked into exactly what's going > > on, > > > > but > > > > > I bet it's due to the skip-not-found behaviour, where we're not caching > it > > > > > properly and it's falling through to multiple reads. > > > > > > > > > > Once that's sorted out we should add tests for it. > > > > > > > > I do believe you're right: the number of not founds is equal to the number > > of > > > > extra reads in cron_servlet_test. > > > > > > Cool. I'll leave it to you. I presume it's an existing bug that was never > > > caught. > > > > I'm pretty sure it's my fault, I introduced more skip_not_found functionality > in > > this patch. Thanks for shedding light on the issue. > > skip_not_found already exists: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... It does, but I added skip_not_found to ReadSingle and CompiledFS. > I guess the tests don't actually check caching behaviour: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > That's a nice patch there. Write a test to demonstrate the caching issue, fix > it, done. Sorry, this isn't ready for review, although it looks like you didn't. I just wanted to check the diff before leaving the office today.
> Sorry, this isn't ready for review, although it looks like you didn't. I just > wanted to check the diff before leaving the office today. Hm when I said "nice patch" I meant in the future tense :)
Patchset #6 (id:180001) has been deleted
Now that the caching issue has been sorted out all is looking good. PTAL.
lgtm
The CQ bit was checked by ahernandez.miralles@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ahernandez.miralles@gmail.com/45371300...
Message was sent while issue was closed.
Committed patchset #6 (id:200001) as f29dd81942aecab73cf5d27978ee544bfd88958c
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/473591b50d408fa2dda99f9b11d326db60a430a1 Cr-Commit-Position: refs/heads/master@{#292223}
Message was sent while issue was closed.
On 2014/09/10 02:54:03, commit-bot: I haz the power wrote: > Patchset 6 (id:??) landed as > https://crrev.com/473591b50d408fa2dda99f9b11d326db60a430a1 > Cr-Commit-Position: refs/heads/master@{#292223} hm, file system provider api is not showing on the list. |
