Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2456603003:
[Predator] Add local cache for get command output. (Closed)
|
Created:
4 years, 1 month ago by Sharu Jiang Modified:
4 years, 1 month ago CC:
chromium-reviews, infra-reviews+infra_chromium.org, chanli, lijeffrey Target Ref:
refs/heads/master Project:
infra Visibility:
Public. |
Description[Predator] Add local cache for get command output.
BUG=605369
Committed: https://chromium.googlesource.com/infra/infra/+/6261f070027929832dd22a109a575733e0db0aee
Patch Set 1 : . #
Total comments: 17
Patch Set 2 : Mock file IO - hold reviewing, fixing a bug. #Patch Set 3 : Fix mock file test. #
Total comments: 2
Patch Set 4 : Rebase and move LocalCacher to util_script/ #
Total comments: 2
Patch Set 5 : Fix nits. #Patch Set 6 : . #
Total comments: 8
Patch Set 7 : Rebase. #
Dependent Patchsets: Messages
Total messages: 34 (16 generated)
Patchset #1 (id:1) has been deleted
katesonia@chromium.org changed reviewers: + mbarbella@chromium.org, wrengr@chromium.org
Description was changed from ========== [Predator] Add local cache for get command output. BUG=605369 ========== to ========== [Predator] Add local cache for get command output. BUG=605369 ==========
katesonia@chromium.org changed reviewers: + stgao@chromium.org
PTAL :)
https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... File appengine/findit/common/cache_decorator.py (right): https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/cache_decorator.py:49: _DEFAULT_LOCAL_CACHE_DIR = os.path.join(os.path.expanduser('~'), '.cache') This shouldn't use such a generic name. This will actually already exist and be used for other things in many environments. Maybe something like ~/.predator/cache. https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/cache_decorator.py:140: class LocalCacher(object): Solely for the sake of consistency, it seems like this should extend Cacher. https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/cache_decorator.py:160: logging.error('Failed load cache.') Could you make this error a bit more descriptive, similar to the one below? https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... File appengine/findit/common/repo_util.py (right): https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/repo_util.py:81: @cache_decorator.Cached('Command-output', cacher=cache_decorator.LocalCacher()) Is the format used here for the namespace consistent with the other uses of Cached? I don't particularly like this format, but I'm fine with it so long as it's consistent with the rest of the usage. https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... File appengine/findit/common/test/cache_decorator_test.py (right): https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/test/cache_decorator_test.py:15: _LOCAL_CACHE_TEST_DIR = os.path.join(os.path.expanduser('~'), '.test_cache') Could you use https://pypi.python.org/pypi/pyfakefs here? I really don't like the idea of having tests actually write to this directory. It can be fairly painful to maintain, and could be nondeterministic between runs for many, many reasons (cleanup and all of the usual IO problems).
https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... File appengine/findit/common/cache_decorator.py (right): https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/cache_decorator.py:49: _DEFAULT_LOCAL_CACHE_DIR = os.path.join(os.path.expanduser('~'), '.cache') On 2016/10/26 23:21:20, Martin Barbella wrote: > This shouldn't use such a generic name. This will actually already exist and be > used for other things in many environments. Maybe something like > ~/.predator/cache. Seconded. https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/cache_decorator.py:52: class Cacher(object): Using "-er" names for classes is weird. Why not just call it "Cache"? https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... File appengine/findit/common/test/repo_util_test.py (right): https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/test/repo_util_test.py:160: self.mock(LocalCacher, 'Get', _MockLocalCacherGet) Why not just use a lambda. It's shorter and cleaner
https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... File appengine/findit/common/cache_decorator.py (right): https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/cache_decorator.py:49: _DEFAULT_LOCAL_CACHE_DIR = os.path.join(os.path.expanduser('~'), '.cache') On 2016/10/27 16:36:47, wrengr wrote: > On 2016/10/26 23:21:20, Martin Barbella wrote: > > This shouldn't use such a generic name. This will actually already exist and > be > > used for other things in many environments. Maybe something like > > ~/.predator/cache. > > Seconded. Done. Use .culprit_finder/local_cache since it's shared code in common. https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/cache_decorator.py:52: class Cacher(object): On 2016/10/27 16:36:47, wrengr wrote: > Using "-er" names for classes is weird. Why not just call it "Cache"? I think it means 'Cacher' caches. https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/cache_decorator.py:140: class LocalCacher(object): On 2016/10/26 23:21:20, Martin Barbella wrote: > Solely for the sake of consistency, it seems like this should extend Cacher. Oops, it should. https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/cache_decorator.py:160: logging.error('Failed load cache.') On 2016/10/26 23:21:20, Martin Barbella wrote: > Could you make this error a bit more descriptive, similar to the one below? Done. https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... File appengine/findit/common/repo_util.py (right): https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/repo_util.py:81: @cache_decorator.Cached('Command-output', cacher=cache_decorator.LocalCacher()) On 2016/10/26 23:21:20, Martin Barbella wrote: > Is the format used here for the namespace consistent with the other uses of > Cached? I don't particularly like this format, but I'm fine with it so long as > it's consistent with the rest of the usage. The other 2 are 'Gitiles-json-view' for SendRequestForJsonResponse and 'Gitiles-text-view' for SendRequestForTextResponse. I think they are consistent or do you have any better name? https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... File appengine/findit/common/test/cache_decorator_test.py (right): https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/test/cache_decorator_test.py:15: _LOCAL_CACHE_TEST_DIR = os.path.join(os.path.expanduser('~'), '.test_cache') On 2016/10/26 23:21:20, Martin Barbella wrote: > Could you use https://pypi.python.org/pypi/pyfakefs here? I really don't like > the idea of having tests actually write to this directory. It can be fairly > painful to maintain, and could be nondeterministic between runs for many, many > reasons (cleanup and all of the usual IO problems). The pyfakefs is third_party libs, use mock instead. https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... File appengine/findit/common/test/repo_util_test.py (right): https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/test/repo_util_test.py:160: self.mock(LocalCacher, 'Get', _MockLocalCacherGet) On 2016/10/27 16:36:47, wrengr wrote: > Why not just use a lambda. It's shorter and cleaner Done.
https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... File appengine/findit/common/cache_decorator.py (right): https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/cache_decorator.py:52: class Cacher(object): On 2016/10/27 21:59:05, sharu jiang wrote: > On 2016/10/27 16:36:47, wrengr wrote: > > Using "-er" names for classes is weird. Why not just call it "Cache"? > > I think it means 'Cacher' caches. In all the communities I've programmed in, we don't subjectize nouns. The base noun is more natural in English. (Just like how we don't call it a "collector" we call it a "collection"; we don't call it a "storer" we call it "storage"; we don't call it an "adder" (outside of circuitry) we call it "addition"; we don't call it a "runner" we call it a "task", or "thread", or "pipeline", or whatever; etc.) There are only a handful of situations where the "-er" form is used: namely where the computational object has taken on a life of its own, outside the the results it produces. For only then do we start thinking about the producer of results as an entity which stands on its own and deserves more focus than the results it produces. The act of caching is very simple, so using "-er" puts emphasis on the wrong thing. "Fetcher" also sounds similarly alien, fwiw.
https://codereview.chromium.org/2456603003/diff/60001/appengine/findit/common... File appengine/findit/common/cache_decorator.py (right): https://codereview.chromium.org/2456603003/diff/60001/appengine/findit/common... appengine/findit/common/cache_decorator.py:146: def __init__(self, cache_dir=_DEFAULT_LOCAL_CACHE_DIR): Can we avoid the default value, and let client pass it over instead?
Patchset #5 (id:100001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
PTAL :) https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... File appengine/findit/common/cache_decorator.py (right): https://codereview.chromium.org/2456603003/diff/20001/appengine/findit/common... appengine/findit/common/cache_decorator.py:52: class Cacher(object): On 2016/10/28 18:02:00, wrengr wrote: > On 2016/10/27 21:59:05, sharu jiang wrote: > > On 2016/10/27 16:36:47, wrengr wrote: > > > Using "-er" names for classes is weird. Why not just call it "Cache"? > > > > I think it means 'Cacher' caches. > > In all the communities I've programmed in, we don't subjectize nouns. The base > noun is more natural in English. (Just like how we don't call it a "collector" > we call it a "collection"; we don't call it a "storer" we call it "storage"; we > don't call it an "adder" (outside of circuitry) we call it "addition"; we don't > call it a "runner" we call it a "task", or "thread", or "pipeline", or whatever; > etc.) There are only a handful of situations where the "-er" form is used: > namely where the computational object has taken on a life of its own, outside > the the results it produces. For only then do we start thinking about the > producer of results as an entity which stands on its own and deserves more focus > than the results it produces. The act of caching is very simple, so using "-er" > puts emphasis on the wrong thing. > > "Fetcher" also sounds similarly alien, fwiw. This make senses, added a todo. https://codereview.chromium.org/2456603003/diff/60001/appengine/findit/common... File appengine/findit/common/cache_decorator.py (right): https://codereview.chromium.org/2456603003/diff/60001/appengine/findit/common... appengine/findit/common/cache_decorator.py:146: def __init__(self, cache_dir=_DEFAULT_LOCAL_CACHE_DIR): On 2016/11/02 02:15:07, stgao (slow on Monday) wrote: > Can we avoid the default value, and let client pass it over instead? Done.
lgtm with nits. https://codereview.chromium.org/2456603003/diff/180001/appengine/findit/util_... File appengine/findit/util_scripts/local_cache.py (right): https://codereview.chromium.org/2456603003/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/local_cache.py:32: logging.error('Failed loading cache: %s', e) Could we use logging.exception instead? Same for below.
Patchset #5 (id:200001) has been deleted
https://codereview.chromium.org/2456603003/diff/180001/appengine/findit/util_... File appengine/findit/util_scripts/local_cache.py (right): https://codereview.chromium.org/2456603003/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/local_cache.py:32: logging.error('Failed loading cache: %s', e) On 2016/11/08 22:00:43, stgao (slow on Monday) wrote: > Could we use logging.exception instead? Same for below. Done.
https://codereview.chromium.org/2456603003/diff/260001/appengine/findit/util_... File appengine/findit/util_scripts/local_cache.py (right): https://codereview.chromium.org/2456603003/diff/260001/appengine/findit/util_... appengine/findit/util_scripts/local_cache.py:20: if not os.path.exists(cache_dir): # pragma: no cover. Again, this sort of code has an inherent race condition. Should instead do ``try: os.makedirs(cache_dir); except OSException as e: if ...e is directory already exists...: pass; else: ...handle actual problems...`` https://codereview.chromium.org/2456603003/diff/260001/appengine/findit/util_... appengine/findit/util_scripts/local_cache.py:27: return None You probably want this case to also do the logging before returning None, yes? If so, then you can drop this whole conditional since the ``except`` block will handle things. https://codereview.chromium.org/2456603003/diff/260001/appengine/findit/util_... File appengine/findit/util_scripts/script_util.py (right): https://codereview.chromium.org/2456603003/diff/260001/appengine/findit/util_... appengine/findit/util_scripts/script_util.py:16: '.command_output_cache') Wait, weren't we putting temp files in ~/.predator/* (or something like that)? Why the change to putting them wherever the script lives?
Patchset #6 (id:240001) has been deleted
https://codereview.chromium.org/2456603003/diff/260001/appengine/findit/util_... File appengine/findit/util_scripts/local_cache.py (right): https://codereview.chromium.org/2456603003/diff/260001/appengine/findit/util_... appengine/findit/util_scripts/local_cache.py:20: if not os.path.exists(cache_dir): # pragma: no cover. On 2016/11/11 18:26:26, wrengr wrote: > Again, this sort of code has an inherent race condition. Should instead do > ``try: os.makedirs(cache_dir); except OSException as e: if ...e is directory > already exists...: pass; else: ...handle actual problems...`` How about using locks? Or there would be a lot exceptions created and thrown out. https://codereview.chromium.org/2456603003/diff/260001/appengine/findit/util_... appengine/findit/util_scripts/local_cache.py:27: return None On 2016/11/11 18:26:26, wrengr wrote: > You probably want this case to also do the logging before returning None, yes? > If so, then you can drop this whole conditional since the ``except`` block will > handle things. Done. https://codereview.chromium.org/2456603003/diff/260001/appengine/findit/util_... File appengine/findit/util_scripts/script_util.py (right): https://codereview.chromium.org/2456603003/diff/260001/appengine/findit/util_... appengine/findit/util_scripts/script_util.py:16: '.command_output_cache') On 2016/11/11 18:26:26, wrengr wrote: > Wait, weren't we putting temp files in ~/.predator/* (or something like that)? > Why the change to putting them wherever the script lives? Oops.. should have put this into the ~/*
Patchset #7 (id:280001) has been deleted
https://codereview.chromium.org/2456603003/diff/260001/appengine/findit/util_... File appengine/findit/util_scripts/local_cache.py (right): https://codereview.chromium.org/2456603003/diff/260001/appengine/findit/util_... appengine/findit/util_scripts/local_cache.py:27: return None On 2016/11/12 01:01:50, Sharu Jiang wrote: > On 2016/11/11 18:26:26, wrengr wrote: > > You probably want this case to also do the logging before returning None, yes? > > If so, then you can drop this whole conditional since the ``except`` block > will > > handle things. > > Done. I added back this, for local cacher won't cache for those None result, which means for a lot of cases the path cannot be found and this would be expected, thus on need to log it.
The CQ bit was checked by katesonia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stgao@chromium.org Link to the patchset: https://codereview.chromium.org/2456603003/#ps320001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Commit it since it has been a long time, feel free to post comments if you still have concerns. I can address in another cl.
Message was sent while issue was closed.
Description was changed from ========== [Predator] Add local cache for get command output. BUG=605369 ========== to ========== [Predator] Add local cache for get command output. BUG=605369 Committed: https://chromium.googlesource.com/infra/infra/+/6261f070027929832dd22a109a575... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:320001) as https://chromium.googlesource.com/infra/infra/+/6261f070027929832dd22a109a575...
Message was sent while issue was closed.
https://codereview.chromium.org/2456603003/diff/260001/appengine/findit/util_... File appengine/findit/util_scripts/local_cache.py (right): https://codereview.chromium.org/2456603003/diff/260001/appengine/findit/util_... appengine/findit/util_scripts/local_cache.py:20: if not os.path.exists(cache_dir): # pragma: no cover. On 2016/11/12 01:01:50, Sharu Jiang wrote: > On 2016/11/11 18:26:26, wrengr wrote: > > Again, this sort of code has an inherent race condition. Should instead do > > ``try: os.makedirs(cache_dir); except OSException as e: if ...e is directory > > already exists...: pass; else: ...handle actual problems...`` > > How about using locks? Or there would be a lot exceptions created and thrown > out. The problem is in (the interaction with) the filesystem, not in Python itself. Using a Python lock won't help because some other process could be the one to create/remove the file/directory. No matter what programming language you're using, by the time the kernel returns with the answer to ``os.path.exists`` that answer is already stale wrt the filesystem itself. A lock within Python would only protect against other threads in the same python process. A lockfile in the filesystem is guaranteed to be shared across all processes, but that's just pushing the problem elsewhere (since you need to make sure to access the lockfile without causing the same race conditions). There's nothing wrong with using ``try:except:`, it's the right tool for the job here, and it's not expensive (especially when compared to the syscalls). The only exceptions we want to discard are the ones where we tried to ``os.makedirs`` but the directory already exists; this is analogous to the implicit ``else: pass`` branch in the current code. All other exceptions should be passed along (just as the current code would raise those same exceptions).
Message was sent while issue was closed.
Patchset #7 (id:300001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
