[Culprit-Finder] Seperate gae related part in cache_decorator and gitile repository to gae_libs/

[Culprit-Finder] Seperate gae related part in cache_decorator and gitile repository to gae_libs/

BUG=660466
Committed: https://chromium.googlesource.com/infra/infra/+/8fbbd99499879846a74fe2f9e312a1e4b1bd7761

[Sharu Jiang, 2016-12-05 23:01:14]

Patchset #1 (id:1) has been deleted

[Sharu Jiang, 2016-12-05 23:01:21]

Patchset #1 (id:20001) has been deleted

[Sharu Jiang, 2016-12-05 23:04:39]

katesonia@chromium.org changed reviewers:
+ stgao@chromium.org, wrengr@chromium.org

[Sharu Jiang, 2016-12-05 23:04:40]

PTAL.

[wrengr, 2016-12-06 21:52:57]

lgtm with nits

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
File appengine/findit/gae_libs/caches.py (right):

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/caches.py:35: The data to be cached would be pickled
and then compressed.
"would" -> "will"

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/caches.py:64: for index in range(0,
len(compressed_data), self.CHUNK_SIZE):
It's better to use xrange rather than range here. No point in allocating a list
since you're just going to walk over it once.

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
File appengine/findit/gae_libs/gitiles/cached_gitiles_repository.py (right):

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/gitiles/cached_gitiles_repository.py:1: # Copyright
2015 The Chromium Authors. All rights reserved.
year should be 2016

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/gitiles/cached_gitiles_repository.py:5: import
functools
I don't think this import is used

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
File appengine/findit/gae_libs/test/caches_test.py (right):

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/test/caches_test.py:1: 
missing copyright header

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/test/caches_test.py:29: """Tests if
``CompressedMemCache`` cache small data (1KB) successfully."""
"cache" -> "caches"

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/test/caches_test.py:37: """App engine memcache can
only cache data < 1MB at one time, so data > 1MB
single-line docstring before the longer explanation.

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/test/caches_test.py:47: """Tests
``CompressedMemCache`` return None when the data is broken."""
-> "Tests that ``CompressedMemCache`` returns None..."

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/handle...
File appengine/findit/handlers/crash/crash_handler.py (right):

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/handle...
appengine/findit/handlers/crash/crash_handler.py:122: there's nothig we can do),
then we will skip creating the pipeline
"nothig" -> "nothing"

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/lib/ca...
File appengine/findit/lib/cache.py (right):

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/lib/ca...
appengine/findit/lib/cache.py:10: TODO: Add a Delete function (default to no-op)
if needed later.
I'd consider settable(-only) caches to be a different interface from
settable-and-deletable caches. The particular subclasses we have so far all
support deletion, but that needn't be true in general.

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/lib/ca...
appengine/findit/lib/cache.py:13: """Returns the cached data for the given key
if available.
There should be more description about what the expected/required behavior of
subclasses is. For example, would an LRU cache be an allowed subclass? That is,
is it required that whenever something has been set it must always be available
for getting, or not?

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/lib/ca...
File appengine/findit/lib/cache_decorator.py (right):

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/lib/ca...
appengine/findit/lib/cache_decorator.py:93: time (up to 0 month). Defaults to 0
-- never expire.
it now defaults to -1 rather than 0...

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/flake/step_mapper.py (right):

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/flake/step_mapper.py:171: )
not sure what Rietveld is trying to say the change is here. I assume it's
removing trailing space, rather than adding some

[Sharu Jiang, 2016-12-06 23:58:17]

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
File appengine/findit/gae_libs/caches.py (right):

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/caches.py:35: The data to be cached would be pickled
and then compressed.
On 2016/12/06 21:52:56, wrengr wrote:
> "would" -> "will"

Done.

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/caches.py:64: for index in range(0,
len(compressed_data), self.CHUNK_SIZE):
On 2016/12/06 21:52:56, wrengr wrote:
> It's better to use xrange rather than range here. No point in allocating a
list
> since you're just going to walk over it once.

Done.

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
File appengine/findit/gae_libs/gitiles/cached_gitiles_repository.py (right):

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/gitiles/cached_gitiles_repository.py:1: # Copyright
2015 The Chromium Authors. All rights reserved.
On 2016/12/06 21:52:56, wrengr wrote:
> year should be 2016

Done.

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/gitiles/cached_gitiles_repository.py:5: import
functools
On 2016/12/06 21:52:56, wrengr wrote:
> I don't think this import is used

Done.

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
File appengine/findit/gae_libs/test/caches_test.py (right):

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/test/caches_test.py:1: 
On 2016/12/06 21:52:57, wrengr wrote:
> missing copyright header

Done.

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/test/caches_test.py:29: """Tests if
``CompressedMemCache`` cache small data (1KB) successfully."""
On 2016/12/06 21:52:57, wrengr wrote:
> "cache" -> "caches"

Done.

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/test/caches_test.py:37: """App engine memcache can
only cache data < 1MB at one time, so data > 1MB
On 2016/12/06 21:52:57, wrengr wrote:
> single-line docstring before the longer explanation.

Done.

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/test/caches_test.py:47: """Tests
``CompressedMemCache`` return None when the data is broken."""
On 2016/12/06 21:52:57, wrengr wrote:
> -> "Tests that ``CompressedMemCache`` returns None..."

Done.

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/handle...
File appengine/findit/handlers/crash/crash_handler.py (right):

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/handle...
appengine/findit/handlers/crash/crash_handler.py:122: there's nothig we can do),
then we will skip creating the pipeline
On 2016/12/06 21:52:57, wrengr wrote:
> "nothig" -> "nothing"

Done.

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/lib/ca...
File appengine/findit/lib/cache.py (right):

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/lib/ca...
appengine/findit/lib/cache.py:10: TODO: Add a Delete function (default to no-op)
if needed later.
On 2016/12/06 21:52:57, wrengr wrote:
> I'd consider settable(-only) caches to be a different interface from
> settable-and-deletable caches. The particular subclasses we have so far all
> support deletion, but that needn't be true in general.

Acknowledged.

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/lib/ca...
appengine/findit/lib/cache.py:13: """Returns the cached data for the given key
if available.
On 2016/12/06 21:52:57, wrengr wrote:
> There should be more description about what the expected/required behavior of
> subclasses is. For example, would an LRU cache be an allowed subclass? That
is,
> is it required that whenever something has been set it must always be
available
> for getting, or not?

Good question, but I think this function just have very little requirements for
its sub classes.

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/lib/ca...
File appengine/findit/lib/cache_decorator.py (right):

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/lib/ca...
appengine/findit/lib/cache_decorator.py:93: time (up to 0 month). Defaults to 0
-- never expire.
On 2016/12/06 21:52:57, wrengr wrote:
> it now defaults to -1 rather than 0...

After rebase this should defaults to 0, old version...

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/flake/step_mapper.py (right):

https://codereview.chromium.org/2557553002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/flake/step_mapper.py:171: )
On 2016/12/06 21:52:57, wrengr wrote:
> not sure what Rietveld is trying to say the change is here. I assume it's
> removing trailing space, rather than adding some

it's removing the newline at the end.

[Sharu Jiang, 2016-12-07 21:00:15]

Description was changed from

==========
[Predator] Seperate gae related part in cache_decorator and gitile repository
from lib to gae_libs/

BUG=660466
==========

to

==========
[Culprit-Finder] Seperate gae related part in cache_decorator and gitile
repository to gae_libs/

BUG=660466
==========

[Sharu Jiang, 2016-12-07 22:08:46]

Description was changed from

==========
[Culprit-Finder] Seperate gae related part in cache_decorator and gitile
repository to gae_libs/

BUG=660466
==========

to

==========
[Culprit-Finder] Seperate gae related part in cache_decorator and gitile
repository to gae_libs/

BUG=660466
TBR=stgao@chromium.org
==========

[Sharu Jiang, 2016-12-07 23:11:47]

Description was changed from

==========
[Culprit-Finder] Seperate gae related part in cache_decorator and gitile
repository to gae_libs/

BUG=660466
TBR=stgao@chromium.org
==========

to

==========
[Culprit-Finder] Seperate gae related part in cache_decorator and gitile
repository to gae_libs/

BUG=660466
==========

[stgao, 2016-12-08 00:34:34]

lgtm with nits.

https://codereview.chromium.org/2557553002/diff/60001/appengine/findit/lib/ca...
File appengine/findit/lib/cache_decorator.py (right):

https://codereview.chromium.org/2557553002/diff/60001/appengine/findit/lib/ca...
appengine/findit/lib/cache_decorator.py:39: import logging
import order.

https://codereview.chromium.org/2557553002/diff/60001/appengine/findit/lib/ca...
appengine/findit/lib/cache_decorator.py:69: expire_time=-1,
Why change to -1? IIRC, memcache use 0 for default.

[Sharu Jiang, 2016-12-08 00:54:31]

https://codereview.chromium.org/2557553002/diff/60001/appengine/findit/lib/ca...
File appengine/findit/lib/cache_decorator.py (right):

https://codereview.chromium.org/2557553002/diff/60001/appengine/findit/lib/ca...
appengine/findit/lib/cache_decorator.py:39: import logging
On 2016/12/08 00:34:34, stgao (slow on Monday) wrote:
> import order.

Done.

https://codereview.chromium.org/2557553002/diff/60001/appengine/findit/lib/ca...
appengine/findit/lib/cache_decorator.py:69: expire_time=-1,
On 2016/12/08 00:34:34, stgao (slow on Monday) wrote:
> Why change to -1? IIRC, memcache use 0 for default.

A rebase problem... didn't mean to change it.

[Sharu Jiang, 2016-12-08 00:55:40]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-12-08 00:55:40]

The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org,
stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2557553002/#ps80001
(title: "Fix nits.")

[commit-bot: I haz the power, 2016-12-08 00:55:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 01:10:55]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481158540534770,
"parent_rev": "b012793fee91810f6aa8a4de645836ed83343bbb", "commit_rev":
"8fbbd99499879846a74fe2f9e312a1e4b1bd7761"}

[commit-bot: I haz the power, 2016-12-08 01:10:58]

Description was changed from

==========
[Culprit-Finder] Seperate gae related part in cache_decorator and gitile
repository to gae_libs/

BUG=660466
==========

to

==========
[Culprit-Finder] Seperate gae related part in cache_decorator and gitile
repository to gae_libs/

BUG=660466

Committed:
https://chromium.googlesource.com/infra/infra/+/8fbbd99499879846a74fe2f9e312a...
==========

[commit-bot: I haz the power, 2016-12-08 01:10:59]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/infra/infra/+/8fbbd99499879846a74fe2f9e312a...