[Findit] Bailing out if build data is too old and moving relevant settings to config

[Findit] Bailing out if build data is too old and moving relevant settings to config

BUG=611189
Committed: https://chromium.googlesource.com/infra/infra/+/458b527a43bcdac9bb02dabc21dbbbd232fea22c

[lijeffrey, 2016-05-20 19:53:13]

Description was changed from

==========
[Findit] Bailing out if build data is too old and moving relevant settings to
config

BUG=
==========

to

==========
[Findit] Bailing out if build data is too old and moving relevant settings to
config

BUG=61189
==========

[lijeffrey, 2016-05-20 19:53:13]

lijeffrey@chromium.org changed reviewers:
+ chanli@chromium.org, stgao@chromium.org

[lijeffrey, 2016-05-20 19:54:20]

ptal

[lijeffrey, 2016-05-20 19:54:40]

Description was changed from

==========
[Findit] Bailing out if build data is too old and moving relevant settings to
config

BUG=61189
==========

to

==========
[Findit] Bailing out if build data is too old and moving relevant settings to
config

BUG=611189
==========

[chanli, 2016-05-20 23:53:54]

https://codereview.chromium.org/1999653003/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/build_util.py (right):

https://codereview.chromium.org/1999653003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/build_util.py:42: if build.data:
Nit: how about changing code from ln42 to 55 to:
if use_cbe:
  build.data = buildbot.GetBuildDataFromArchive(
      master_name, builder_name, build_number, HTTP_CLIENT_NO_404_ERROR)

  if build.data is not None:
     build.data_source = CHROME_BUILD_EXTRACT
  else:
    if not lock_util.WaitUntilDownloadAllowed(master_name):
      return None
    build.data = buildbot.GetBuildDataFromBuildMaster(
        master_name, builder_name, build_number, HTTP_CLIENT_LOGGING_ERRORS)
     build.data_source = BUILDBOT_MASTER
    build.last_crawled_time = datetime.utcnow()
    build.put()

https://codereview.chromium.org/1999653003/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/waterfall_config.py (right):

https://codereview.chromium.org/1999653003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/waterfall_config.py:168: 
nit: empty line

[lijeffrey, 2016-05-21 01:50:20]

https://codereview.chromium.org/1999653003/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/build_util.py (right):

https://codereview.chromium.org/1999653003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/build_util.py:42: if build.data:
On 2016/05/20 23:53:54, chanli wrote:
> Nit: how about changing code from ln42 to 55 to:
> if use_cbe:
>   build.data = buildbot.GetBuildDataFromArchive(
>       master_name, builder_name, build_number, HTTP_CLIENT_NO_404_ERROR)
> 
>   if build.data is not None:
>      build.data_source = CHROME_BUILD_EXTRACT
>   else:
>     if not lock_util.WaitUntilDownloadAllowed(master_name):
>       return None
>     build.data = buildbot.GetBuildDataFromBuildMaster(
>         master_name, builder_name, build_number, HTTP_CLIENT_LOGGING_ERRORS)
>      build.data_source = BUILDBOT_MASTER
>     build.last_crawled_time = datetime.utcnow()
>     build.put()

we still want to fallback to buildmaster even if use_cbe is True but does not
return results. The code as it is also allows for 1 write for build.put()

[chanli, 2016-05-21 02:52:23]

On 2016/05/21 01:50:20, lijeffrey wrote:
>
https://codereview.chromium.org/1999653003/diff/1/appengine/findit/waterfall/...
> File appengine/findit/waterfall/build_util.py (right):
> 
>
https://codereview.chromium.org/1999653003/diff/1/appengine/findit/waterfall/...
> appengine/findit/waterfall/build_util.py:42: if build.data:
> On 2016/05/20 23:53:54, chanli wrote:
> > Nit: how about changing code from ln42 to 55 to:
> > if use_cbe:
> >   build.data = buildbot.GetBuildDataFromArchive(
> >       master_name, builder_name, build_number, HTTP_CLIENT_NO_404_ERROR)
> > 
> >   if build.data is not None:
> >      build.data_source = CHROME_BUILD_EXTRACT
> >   else:
> >     if not lock_util.WaitUntilDownloadAllowed(master_name):
> >       return None
> >     build.data = buildbot.GetBuildDataFromBuildMaster(
> >         master_name, builder_name, build_number, HTTP_CLIENT_LOGGING_ERRORS)
> >      build.data_source = BUILDBOT_MASTER
> >     build.last_crawled_time = datetime.utcnow()
> >     build.put()
> 
> we still want to fallback to buildmaster even if use_cbe is True but does not
> return results. The code as it is also allows for 1 write for build.put()

lgtm

[stgao, 2016-05-21 04:26:41]

https://codereview.chromium.org/1999653003/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/test/try_job_util_test.py (right):

https://codereview.chromium.org/1999653003/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/test/try_job_util_test.py:74: build.start_time =
datetime.utcnow()
This might make the test flaky as it depends on the OS clock.

See my comment below.

https://codereview.chromium.org/1999653003/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/try_job_util.py (right):

https://codereview.chromium.org/1999653003/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/try_job_util.py:162: if (datetime.utcnow() -
build.start_time).days > 0:
Move the check to a separate function, and it could be mocked in the test.

[lijeffrey, 2016-05-21 09:02:52]

https://codereview.chromium.org/1999653003/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/try_job_util.py (right):

https://codereview.chromium.org/1999653003/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/try_job_util.py:162: if (datetime.utcnow() -
build.start_time).days > 0:
On 2016/05/21 04:26:41, stgao wrote:
> Move the check to a separate function, and it could be mocked in the test.

Done.

[lijeffrey, 2016-05-21 09:02:57]

The CQ bit was checked by lijeffrey@chromium.org

[lijeffrey, 2016-05-21 09:02:57]

The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org
Link to the patchset: https://codereview.chromium.org/1999653003/#ps40001
(title: "Addressing comments")

[commit-bot: I haz the power, 2016-05-21 09:03:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1999653003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1999653003/40001

[commit-bot: I haz the power, 2016-05-21 09:07:05]

Description was changed from

==========
[Findit] Bailing out if build data is too old and moving relevant settings to
config

BUG=611189
==========

to

==========
[Findit] Bailing out if build data is too old and moving relevant settings to
config

BUG=611189

Committed:
https://chromium.googlesource.com/infra/infra/+/458b527a43bcdac9bb02dabc21dbb...
==========

[commit-bot: I haz the power, 2016-05-21 09:07:07]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/infra/infra/+/458b527a43bcdac9bb02dabc21dbb...