[Predator] Run predator.

[Predator] Run predator.

Implement run-predator.py and make delta test script run.

BUG=605369
Committed: https://chromium.googlesource.com/infra/infra/+/ced5e9b61c62f348d5e880d1cec54f0ba1fe6309

[Sharu Jiang, 2016-10-31 22:58:48]

Patchset #2 (id:20001) has been deleted

[Sharu Jiang, 2016-10-31 22:58:55]

Patchset #2 (id:40001) has been deleted

[Sharu Jiang, 2016-10-31 22:59:01]

Patchset #2 (id:60001) has been deleted

[Sharu Jiang, 2016-10-31 22:59:10]

Patchset #1 (id:1) has been deleted

[Sharu Jiang, 2016-10-31 23:10:06]

Patchset #1 (id:80001) has been deleted

[Sharu Jiang, 2016-10-31 23:13:58]

Patchset #1 (id:100001) has been deleted

[Sharu Jiang, 2016-10-31 23:14:04]

Patchset #1 (id:120001) has been deleted

[Sharu Jiang, 2016-10-31 23:16:17]

Description was changed from

==========
[Predator] Run predator.

BUG=605369
==========

to

==========
[Predator] Run predator.

Implement run-predator.py and make delta test script run.

BUG=605369
==========

[Sharu Jiang, 2016-10-31 23:16:17]

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

[Sharu Jiang, 2016-10-31 23:30:34]

Patchset #1 (id:140001) has been deleted

[Sharu Jiang, 2016-11-01 00:29:19]

PTAL :)

[wrengr, 2016-11-01 22:07:52]

lgtm, with nits.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/culprit.py (right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/crash...
appengine/findit/crash/culprit.py:101: result['regression_range'] =
self.regression_range
We should copy the self.regression_range into a list, since it may be (i.e.,
should be) a tuple but comparing tuples to lists fails in the unittests.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/lib/g...
File appengine/findit/lib/gitiles/test/repo_util_test.py (right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/lib/g...
appengine/findit/lib/gitiles/test/repo_util_test.py:149: if self.command ==
'dummy':
nit: would be cleaner to ``return 1 if ... else 0``

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/lib/g...
appengine/findit/lib/gitiles/test/repo_util_test.py:154: def _MockPopen(command,
**_):
nit: would be cleaner to inline this as a lambda.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
File appengine/findit/util_scripts/crash_queries/crash_iterator.py (right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/crash_iterator.py:22: def
GetAnalysisClassForClient(client_id):
For future reference: You should be able to replace this implementation with
``FinditForClientID(client_id).GetAnalysis`` (i.e., returning that method as a
function object, not calling that method). Feel free to do that in a separate
CL.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
File appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py
(right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:82: print
traceback.format_exc()
Shouldn't we throw an exception or return something to the caller indicate that
things failed?

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:88:
origin_get = CrashConfig.Get
This line should be moved outside of the try block, so that origin_get is
guaranteed to be defined in the finally block.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:119:
argparser.add_argument('app', help=('App engine id to get config from.'))
nit: unnecessary parentheses around the string.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
File appengine/findit/util_scripts/iterator.py (right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/iterator.py:39: return complete entities.
What do you mean "return complete entities." ?

[stgao, 2016-11-02 01:45:49]

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/lib/c...
File appengine/findit/lib/cache_decorator.py (right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/lib/c...
appengine/findit/lib/cache_decorator.py:51: _PREDATOR_ROOT_DIR =
os.path.join(os.path.dirname(__file__), os.path.pardir)
not "PREDATOR" here, as this is for general purpose.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
File appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py
(right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:35:
'https://crash-frontend-dot-%s.appspot.com/'
Why we have to set the module "crash-frontend" explicitly here?

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:39: def
GetCrashConfigFromRemoteAPI(cls, app_id, **_):
What's ``cls`` here?

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:49:
return config
Why we retrieve the latest version of config in this way? Can't we use the
existing API?

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:114:
description='Run azalea on a batch of crashes.')
azalea

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
File appengine/findit/util_scripts/script_util.py (right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/script_util.py:41: def SignalWorkerThreads():
Is this a copy from the version in ClusterFuzz? Or are there some significant
modification?

[Sharu Jiang, 2016-11-11 23:10:26]

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/culprit.py (right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/crash...
appengine/findit/crash/culprit.py:101: result['regression_range'] =
self.regression_range
On 2016/11/01 22:07:52, wrengr wrote:
> We should copy the self.regression_range into a list, since it may be (i.e.,
> should be) a tuple but comparing tuples to lists fails in the unittests.

Done in rebase.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/lib/c...
File appengine/findit/lib/cache_decorator.py (right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/lib/c...
appengine/findit/lib/cache_decorator.py:51: _PREDATOR_ROOT_DIR =
os.path.join(os.path.dirname(__file__), os.path.pardir)
On 2016/11/02 01:45:48, stgao (slow on Monday) wrote:
> not "PREDATOR" here, as this is for general purpose.

Done in rebase.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/lib/g...
File appengine/findit/lib/gitiles/test/repo_util_test.py (right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/lib/g...
appengine/findit/lib/gitiles/test/repo_util_test.py:149: if self.command ==
'dummy':
On 2016/11/01 22:07:52, wrengr wrote:
> nit: would be cleaner to ``return 1 if ... else 0``

Done.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/lib/g...
appengine/findit/lib/gitiles/test/repo_util_test.py:154: def _MockPopen(command,
**_):
On 2016/11/01 22:07:52, wrengr wrote:
> nit: would be cleaner to inline this as a lambda.

Done.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
File appengine/findit/util_scripts/crash_queries/crash_iterator.py (right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/crash_iterator.py:22: def
GetAnalysisClassForClient(client_id):
On 2016/11/01 22:07:52, wrengr wrote:
> For future reference: You should be able to replace this implementation with
> ``FinditForClientID(client_id).GetAnalysis`` (i.e., returning that method as a
> function object, not calling that method). Feel free to do that in a separate
> CL.

This one is to return Class instead of instance, some refators are needed, will
do this in another cl.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
File appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py
(right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:35:
'https://crash-frontend-dot-%s.appspot.com/'
On 2016/11/02 01:45:48, stgao (slow on Monday) wrote:
> Why we have to set the module "crash-frontend" explicitly here?

Oops, testing purpose.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:39: def
GetCrashConfigFromRemoteAPI(cls, app_id, **_):
On 2016/11/02 01:45:48, stgao (slow on Monday) wrote:
> What's ``cls`` here?

It means the cls, since this is to replace class method of CrashConfig.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:49:
return config
On 2016/11/02 01:45:49, stgao (slow on Monday) wrote:
> Why we retrieve the latest version of config in this way? Can't we use the
> existing API?

CrashConfig.Get() failed and I cannot find a way to work around it.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:82: print
traceback.format_exc()
On 2016/11/01 22:07:52, wrengr wrote:
> Shouldn't we throw an exception or return something to the caller indicate
that
> things failed?

Done.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:88:
origin_get = CrashConfig.Get
On 2016/11/01 22:07:52, wrengr wrote:
> This line should be moved outside of the try block, so that origin_get is
> guaranteed to be defined in the finally block.

Done.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:114:
description='Run azalea on a batch of crashes.')
On 2016/11/02 01:45:48, stgao (slow on Monday) wrote:
> azalea

Done.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:119:
argparser.add_argument('app', help=('App engine id to get config from.'))
On 2016/11/01 22:07:52, wrengr wrote:
> nit: unnecessary parentheses around the string.

Done.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
File appengine/findit/util_scripts/iterator.py (right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/iterator.py:39: return complete entities.
On 2016/11/01 22:07:52, wrengr wrote:
> What do you mean "return complete entities." ?

I mean model instead of a projected dict.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
File appengine/findit/util_scripts/script_util.py (right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/script_util.py:41: def SignalWorkerThreads():
On 2016/11/02 01:45:49, stgao (slow on Monday) wrote:
> Is this a copy from the version in ClusterFuzz? Or are there some significant
> modification?

It is a copy from the Clusterfuzz.

[Sharu Jiang, 2016-11-14 20:56:14]

Patchset #4 (id:220001) has been deleted

[Sharu Jiang, 2016-11-14 21:16:22]

Patchset #5 (id:260001) has been deleted

[stgao, 2016-11-14 21:34:12]

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
File appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py
(right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:39: def
GetCrashConfigFromRemoteAPI(cls, app_id, **_):
On 2016/11/11 23:10:26, Sharu Jiang wrote:
> On 2016/11/02 01:45:48, stgao (slow on Monday) wrote:
> > What's ``cls`` here?
> 
> It means the cls, since this is to replace class method of CrashConfig.

Sorry, I still don't get it. Could we add a docstring for the parameters
including their types?

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:49:
return config
On 2016/11/11 23:10:26, Sharu Jiang wrote:
> On 2016/11/02 01:45:49, stgao (slow on Monday) wrote:
> > Why we retrieve the latest version of config in this way? Can't we use the
> > existing API?
> 
> CrashConfig.Get() failed and I cannot find a way to work around it.

What failure it is? Why it failed? Can we fix it instead of working around it?

https://codereview.chromium.org/2432203003/diff/280001/appengine/findit/crash...
File appengine/findit/crash/test/crash_testcase.py (right):

https://codereview.chromium.org/2432203003/diff/280001/appengine/findit/crash...
appengine/findit/crash/test/crash_testcase.py:108: # and this helps ensure that.
what's this for?

https://codereview.chromium.org/2432203003/diff/280001/appengine/findit/crash...
appengine/findit/crash/test/crash_testcase.py:114: return MockHttpClient()
Why not just use a unittest.mock.Mock object?

[wrengr, 2016-11-14 21:51:07]

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
File appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py
(right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:49:
return config
On 2016/11/14 21:34:12, stgao (slow on Monday) wrote:
> On 2016/11/11 23:10:26, Sharu Jiang wrote:
> > On 2016/11/02 01:45:49, stgao (slow on Monday) wrote:
> > > Why we retrieve the latest version of config in this way? Can't we use the
> > > existing API?
> > 
> > CrashConfig.Get() failed and I cannot find a way to work around it.
> 
> What failure it is? Why it failed? Can we fix it instead of working around it?

At the very least, there should be a comment (and preferably a bug ticket)
explaining the problem.

[Sharu Jiang, 2016-11-15 02:21:47]

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
File appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py
(right):

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:39: def
GetCrashConfigFromRemoteAPI(cls, app_id, **_):
On 2016/11/14 21:34:12, stgao (slow on Monday) wrote:
> On 2016/11/11 23:10:26, Sharu Jiang wrote:
> > On 2016/11/02 01:45:48, stgao (slow on Monday) wrote:
> > > What's ``cls`` here?
> > 
> > It means the cls, since this is to replace class method of CrashConfig.
> 
> Sorry, I still don't get it. Could we add a docstring for the parameters
> including their types?

Sure, done.

https://codereview.chromium.org/2432203003/diff/160001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:49:
return config
On 2016/11/14 21:51:07, wrengr wrote:
> On 2016/11/14 21:34:12, stgao (slow on Monday) wrote:
> > On 2016/11/11 23:10:26, Sharu Jiang wrote:
> > > On 2016/11/02 01:45:49, stgao (slow on Monday) wrote:
> > > > Why we retrieve the latest version of config in this way? Can't we use
the
> > > > existing API?
> > > 
> > > CrashConfig.Get() failed and I cannot find a way to work around it.
> > 
> > What failure it is? Why it failed? Can we fix it instead of working around
it?
> 
> At the very least, there should be a comment (and preferably a bug ticket)
> explaining the problem.


Traceback (most recent call last):
  File "run-predator.py", line 150, in <module>
    RunPredator()
  File "run-predator.py", line 133, in RunPredator
    culprits = GetCulprits(crashes, args.client, args.app, args.verbose)
  File "run-predator.py", line 93, in GetCulprits
    findit = FinditForClientID(client_id, LocalGitRepository())
  File "../../../crash/crash_pipeline.py", line 53, in FinditForClientID
    return cls(repository)
  File "../../../crash/findit_for_chromecrash.py", line 54, in __init__
    component_classifier_config = CrashConfig.Get().component_classifier
  File "../../../model/versioned_config.py", line 29, in Get
    config_data = cls.GetVersion(version=version)
  File "../../../model/versioned_model.py", line 68, in GetVersion
    root = root_key.get()
  File
"/usr/local/google/home/katesonia/Workstation/chromium_infra/google_appengine/google/appengine/ext/ndb/key.py",
line 572, in get
    return self.get_async(**ctx_options).get_result()
  File
"/usr/local/google/home/katesonia/Workstation/chromium_infra/google_appengine/google/appengine/ext/ndb/tasklets.py",
line 383, in get_result
    self.check_success()
  File
"/usr/local/google/home/katesonia/Workstation/chromium_infra/google_appengine/google/appengine/ext/ndb/tasklets.py",
line 427, in _help_tasklet_along
    value = gen.throw(exc.__class__, exc, tb)
  File
"/usr/local/google/home/katesonia/Workstation/chromium_infra/google_appengine/google/appengine/ext/ndb/context.py",
line 722, in get
    deadline=memcache_deadline)
  File
"/usr/local/google/home/katesonia/Workstation/chromium_infra/google_appengine/google/appengine/ext/ndb/tasklets.py",
line 430, in _help_tasklet_along
    value = gen.send(val)
  File
"/usr/local/google/home/katesonia/Workstation/chromium_infra/google_appengine/google/appengine/ext/ndb/context.py",
line 1104, in _memcache_get_tasklet
    rpc = memcache.create_rpc(deadline=deadline)
  File
"/usr/local/google/home/katesonia/Workstation/chromium_infra/google_appengine/google/appengine/api/memcache/__init__.py",
line 294, in create_rpc
    return apiproxy_stub_map.UserRPC('memcache', deadline, callback)
  File
"/usr/local/google/home/katesonia/Workstation/chromium_infra/google_appengine/google/appengine/api/apiproxy_stub_map.py",
line 414, in __init__
    self.__rpc = CreateRPC(service, stubmap)
  File
"/usr/local/google/home/katesonia/Workstation/chromium_infra/google_appengine/google/appengine/api/apiproxy_stub_map.py",
line 68, in CreateRPC
    assert stub, 'No api proxy found for service "%s"' % service
AssertionError: No api proxy found for service "memcache"

Bug created with the full stacktrace.

So far from the search I did, the ``remote_api`` is the only way to access
production datastore from local script. I cannot find a way to do key.get() for
model in datastore without this remote_api work around.

https://codereview.chromium.org/2432203003/diff/280001/appengine/findit/crash...
File appengine/findit/crash/test/crash_testcase.py (right):

https://codereview.chromium.org/2432203003/diff/280001/appengine/findit/crash...
appengine/findit/crash/test/crash_testcase.py:108: # and this helps ensure that.
On 2016/11/14 21:34:12, stgao (slow on Monday) wrote:
> what's this for?

This was accidentally pasted... deleted in following patch.

https://codereview.chromium.org/2432203003/diff/280001/appengine/findit/crash...
appengine/findit/crash/test/crash_testcase.py:114: return MockHttpClient()
On 2016/11/14 21:34:12, stgao (slow on Monday) wrote:
> Why not just use a unittest.mock.Mock object?

I think it's better that MockHttpClient is a instance of RetryHttpClient.

[Sharu Jiang, 2016-11-16 06:43:36]

Patchset #9 (id:360001) has been deleted

[Sharu Jiang, 2016-11-16 06:43:46]

Patchset #9 (id:380001) has been deleted

[Sharu Jiang, 2016-11-16 06:44:09]

Patchset #8 (id:340001) has been deleted

[Sharu Jiang, 2016-11-17 03:06:03]

Patchset #9 (id:420001) has been deleted

[Sharu Jiang, 2016-11-17 03:06:11]

Patchset #9 (id:440001) has been deleted

[Sharu Jiang, 2016-11-17 03:08:56]

Ping :)

[stgao, 2016-11-17 05:03:05]

lgtm as I don't want to block this CL for too long.

However, there are still quite a few places to be improved.
For easier ones, I suggest fix them in this CL; for harder ones, I'm OK with
follow-up CLs.

BTW, I didn't receive a code review request for patchset 8. You may forget to
publish before you sent the ping :)

https://codereview.chromium.org/2432203003/diff/280001/appengine/findit/crash...
File appengine/findit/crash/test/crash_testcase.py (right):

https://codereview.chromium.org/2432203003/diff/280001/appengine/findit/crash...
appengine/findit/crash/test/crash_testcase.py:114: return MockHttpClient()
On 2016/11/15 02:21:47, Sharu Jiang wrote:
> On 2016/11/14 21:34:12, stgao (slow on Monday) wrote:
> > Why not just use a unittest.mock.Mock object?
> 
> I think it's better that MockHttpClient is a instance of RetryHttpClient.

Why it is better? What's the benefit? Why a unittest.mock.Mock is insufficient?

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/crash...
File appengine/findit/crash/changelist_classifier.py (right):

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/crash...
appengine/findit/crash/changelist_classifier.py:301: repository.repo_url =
stack_deps[dep].repo_url
Why we want this to be in the inner for loop? Does stack_deps[dep].repo_url
change at all?

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
File appengine/findit/util_scripts/crash_queries/delta_test/delta_util.py
(right):

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/delta_util.py:95: print
'Writing delta diff to %s\n' % file_path
Why we switch from logging to print? I thought logging will make it easier to
control the level of logging.

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
File appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py
(right):

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:59: if
verbose:
Why the print have to be protected under the lock? what's the lock for?

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:59: if
verbose:
Instead of a verbose flag, could we use logging level instead?

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:66: print
'\n\nCrash:', crash_url
same here. Why under lock?

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:78:
CrashConfig.Get = script_util.GetLockedMethod(CrashConfig, origin)
We'd better refactor the read of config out to an upper level of code, so that
we could read it once and use it multiple times or in multithread.

I'm fine with this hack to commit this CL as a runnable version.
But this should be fixed.

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
File appengine/findit/util_scripts/script_util.py (right):

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
appengine/findit/util_scripts/script_util.py:157: return method(*args, **kwargs)
This is thread safe if the lock is set appropriately.
But #153-154 is not thread safe.

[Sharu Jiang, 2016-11-17 09:03:16]

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/crash...
File appengine/findit/crash/changelist_classifier.py (right):

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/crash...
appengine/findit/crash/changelist_classifier.py:301: repository.repo_url =
stack_deps[dep].repo_url
On 2016/11/17 05:03:05, stgao (slow on Monday) wrote:
> Why we want this to be in the inner for loop? Does stack_deps[dep].repo_url
> change at all?

Since for many cases, the repo_url doesn't need to be changed if no matches are
found, especially for local_git_repository, changing repo_url is more expensive.

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
File appengine/findit/util_scripts/crash_queries/delta_test/delta_util.py
(right):

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/delta_util.py:95: print
'Writing delta diff to %s\n' % file_path
On 2016/11/17 05:03:05, stgao (slow on Monday) wrote:
> Why we switch from logging to print? I thought logging will make it easier to
> control the level of logging.

The problem is that we have tons of loggings inside findit, things will get
messed up.
Switch to print for now, if later we want to have better log control, we can
implement a simple local logger to have different log level.

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
File appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py
(right):

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:59: if
verbose:
On 2016/11/17 05:03:05, stgao (slow on Monday) wrote:
> Why the print have to be protected under the lock? what's the lock for?

This is to make sure the prints from different threads won't be messed up.

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:59: if
verbose:
On 2016/11/17 05:03:05, stgao (slow on Monday) wrote:
> Instead of a verbose flag, could we use logging level instead?

This is because logging will be messed up with those inside findit.
I created a bug to implement a simple version of logger to have this level
control.

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:66: print
'\n\nCrash:', crash_url
On 2016/11/17 05:03:05, stgao (slow on Monday) wrote:
> same here. Why under lock?

Same as above.

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:78:
CrashConfig.Get = script_util.GetLockedMethod(CrashConfig, origin)
On 2016/11/17 05:03:05, stgao (slow on Monday) wrote:
> We'd better refactor the read of config out to an upper level of code, so that
> we could read it once and use it multiple times or in multithread.
> 
> I'm fine with this hack to commit this CL as a runnable version.
> But this should be fixed.

I think we have a bug(crbug.com/659354), after this is done, we can update the
code.

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
File appengine/findit/util_scripts/script_util.py (right):

https://codereview.chromium.org/2432203003/diff/460001/appengine/findit/util_...
appengine/findit/util_scripts/script_util.py:157: return method(*args, **kwargs)
On 2016/11/17 05:03:05, stgao (slow on Monday) wrote:
> This is thread safe if the lock is set appropriately.
> But #153-154 is not thread safe.

Refactor this, since this is more general function, should have a lock passed.
New version should be thread safe.

[Sharu Jiang, 2016-11-17 09:07:52]

Patchset #10 (id:480001) has been deleted

[stgao, 2016-11-17 15:28:07]

lgtm

[Sharu Jiang, 2016-11-17 20:15:04]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-11-17 20:15:05]

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/2432203003/#ps520001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-11-17 20:15:14]

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

[Sharu Jiang, 2016-11-17 20:16:54]

The CQ bit was unchecked by katesonia@chromium.org

[Sharu Jiang, 2016-11-17 20:16:54]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-11-17 20:16:55]

The CQ bit was unchecked by katesonia@chromium.org

[Sharu Jiang, 2016-11-17 20:21:08]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-11-17 20:21:09]

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/2432203003/#ps540001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-11-17 20:21:13]

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-11-17 20:37:00]

Description was changed from

==========
[Predator] Run predator.

Implement run-predator.py and make delta test script run.

BUG=605369
==========

to

==========
[Predator] Run predator.

Implement run-predator.py and make delta test script run.

BUG=605369

Committed:
https://chromium.googlesource.com/infra/infra/+/ced5e9b61c62f348d5e880d1cec54...
==========

[commit-bot: I haz the power, 2016-11-17 20:37:02]

Committed patchset #12 (id:540001) as
https://chromium.googlesource.com/infra/infra/+/ced5e9b61c62f348d5e880d1cec54...

[Sharu Jiang, 2016-11-18 20:13:20]

Patchset #11 (id:520001) has been deleted

[Sharu Jiang, 2017-03-01 23:50:34]

Patchset #2 (id:180001) has been deleted

[Sharu Jiang, 2017-03-03 01:49:44]

Patchset #3 (id:240001) has been deleted