[Findit] Refactoring WfTryJobData into BaseTryJobData, WfTryJobData, and FlakeTryJobData

[Findit] Refactoring WfTryJobData into BaseTryJobData, WfTryJobData, and FlakeTryJobData

In preparation for triggering flake try jobs, the corresponding try job data entity needs to be refactored to separate WfTryJob from FlakeTryJob

BUG=672724
Review-Url: https://codereview.chromium.org/2605803002
Committed: https://chromium.googlesource.com/infra/infra/+/33ffa3ac64e76a71df2f565bd98b144c17bbafd1

[lijeffrey, 2016-12-28 00:40:17]

Description was changed from

==========
[Findit] Refactoring WfTryJobData into BaseTryJobData, WfTryJobData, and
FlakeTryJobData


[Findit] Get full build info for each flake build analyzed

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

to

==========
[Findit] Refactoring WfTryJobData into BaseTryJobData, WfTryJobData, and
FlakeTryJobData

Note this CL includes changes from https://codereview.chromium.org/2597373002/,
though unrelated, which needs to be committed first.

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

[lijeffrey, 2016-12-28 00:40:17]

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

[lijeffrey, 2016-12-28 00:40:56]

Description was changed from

==========
[Findit] Refactoring WfTryJobData into BaseTryJobData, WfTryJobData, and
FlakeTryJobData

Note this CL includes changes from https://codereview.chromium.org/2597373002/,
though unrelated, which needs to be committed first.

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

to

==========
[Findit] Refactoring WfTryJobData into BaseTryJobData, WfTryJobData, and
FlakeTryJobData

In preparation for triggering flake try jobs, the corresponding try job data
entity needs to be refactored to separate WfTryJob from FlakeTryJob

Note this CL includes changes from https://codereview.chromium.org/2597373002/,
though unrelated, which needs to be committed first.

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

[lijeffrey, 2016-12-28 04:26:57]

Description was changed from

==========
[Findit] Refactoring WfTryJobData into BaseTryJobData, WfTryJobData, and
FlakeTryJobData

In preparation for triggering flake try jobs, the corresponding try job data
entity needs to be refactored to separate WfTryJob from FlakeTryJob

Note this CL includes changes from https://codereview.chromium.org/2597373002/,
though unrelated, which needs to be committed first.

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

to

==========
[Findit] Refactoring WfTryJobData into BaseTryJobData, WfTryJobData, and
FlakeTryJobData

In preparation for triggering flake try jobs, the corresponding try job data
entity needs to be refactored to separate WfTryJob from FlakeTryJob

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

[lijeffrey, 2016-12-28 04:55:35]

ptal

[lijeffrey, 2016-12-30 00:24:16]

Description was changed from

==========
[Findit] Refactoring WfTryJobData into BaseTryJobData, WfTryJobData, and
FlakeTryJobData

In preparation for triggering flake try jobs, the corresponding try job data
entity needs to be refactored to separate WfTryJob from FlakeTryJob

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

to

==========
[Findit] Refactoring WfTryJobData into BaseTryJobData, WfTryJobData, and
FlakeTryJobData

In preparation for triggering flake try jobs, the corresponding try job data
entity needs to be refactored to separate WfTryJob from FlakeTryJob

BUG=672724
==========

[stgao, 2017-01-05 21:15:11]

lgtm % comments

https://codereview.chromium.org/2605803002/diff/60001/appengine/findit/model/...
File appengine/findit/model/base_try_job_data.py (right):

https://codereview.chromium.org/2605803002/diff/60001/appengine/findit/model/...
appengine/findit/model/base_try_job_data.py:9: """Represents a tryjob's data for
a completed try job."""
nit: not necessarily completed?

https://codereview.chromium.org/2605803002/diff/60001/appengine/findit/model/...
File appengine/findit/model/flake/flake_try_job_data.py (right):

https://codereview.chromium.org/2605803002/diff/60001/appengine/findit/model/...
appengine/findit/model/flake/flake_try_job_data.py:13: """Represents a tryjob's
data for a completed compile or test try job."""
This description is incorrect.

https://codereview.chromium.org/2605803002/diff/60001/appengine/findit/model/...
appengine/findit/model/flake/flake_try_job_data.py:15: @property
Should we do ComputedProperty for easier query?

https://codereview.chromium.org/2605803002/diff/60001/appengine/findit/model/...
appengine/findit/model/flake/flake_try_job_data.py:17: return
self.try_job_key.pairs()[0][1].split('/')[0]
Should we move the decoding to the FlakeTryJob model for encapsulation of logic?

https://codereview.chromium.org/2605803002/diff/60001/appengine/findit/model/...
File appengine/findit/model/wf_try_job_data.py (right):

https://codereview.chromium.org/2605803002/diff/60001/appengine/findit/model/...
appengine/findit/model/wf_try_job_data.py:30: @property
Go with @ComputedProperty so that new data works the same as old data?

[lijeffrey, 2017-01-06 04:22:21]

Comments addressed, mind another look over? Especially in FlakeTryJob.py where I
added GetMasterName, GetBuilderName etc to keep the key computations internal to
FlakeTryJob.py.

Thanks,
Jeff

https://codereview.chromium.org/2605803002/diff/60001/appengine/findit/model/...
File appengine/findit/model/base_try_job_data.py (right):

https://codereview.chromium.org/2605803002/diff/60001/appengine/findit/model/...
appengine/findit/model/base_try_job_data.py:9: """Represents a tryjob's data for
a completed try job."""
On 2017/01/05 21:15:11, stgao (slow on Monday) wrote:
> nit: not necessarily completed?

Done.

https://codereview.chromium.org/2605803002/diff/60001/appengine/findit/model/...
File appengine/findit/model/flake/flake_try_job_data.py (right):

https://codereview.chromium.org/2605803002/diff/60001/appengine/findit/model/...
appengine/findit/model/flake/flake_try_job_data.py:13: """Represents a tryjob's
data for a completed compile or test try job."""
On 2017/01/05 21:15:11, stgao (slow on Monday) wrote:
> This description is incorrect.

Oops, copy paste error :)

https://codereview.chromium.org/2605803002/diff/60001/appengine/findit/model/...
appengine/findit/model/flake/flake_try_job_data.py:15: @property
On 2017/01/05 21:15:11, stgao (slow on Monday) wrote:
> Should we do ComputedProperty for easier query?

Done.

https://codereview.chromium.org/2605803002/diff/60001/appengine/findit/model/...
File appengine/findit/model/wf_try_job_data.py (right):

https://codereview.chromium.org/2605803002/diff/60001/appengine/findit/model/...
appengine/findit/model/wf_try_job_data.py:30: @property
On 2017/01/05 21:15:11, stgao (slow on Monday) wrote:
> Go with @ComputedProperty so that new data works the same as old data?

Done.

[lijeffrey, 2017-01-06 04:22:22]

Comments addressed, mind another look over? Especially in FlakeTryJob.py where I
added GetMasterName, GetBuilderName etc to keep the key computations internal to
FlakeTryJob.py.

Thanks,
Jeff

[chanli, 2017-01-07 02:39:39]

https://codereview.chromium.org/2605803002/diff/80001/appengine/findit/model/...
File appengine/findit/model/wf_try_job_data.py (right):

https://codereview.chromium.org/2605803002/diff/80001/appengine/findit/model/...
appengine/findit/model/wf_try_job_data.py:31: def master_name(self):
Since both wf_try_job and flake_try_job have master_name, builder_name and
build_number, can we move these to base_try_job?

https://codereview.chromium.org/2605803002/diff/80001/appengine/findit/model/...
appengine/findit/model/wf_try_job_data.py:31: def master_name(self):
Not sure if we should have similar implementation as flake_try_job_data (move
the decoding to TryJob model)

https://codereview.chromium.org/2605803002/diff/80001/appengine/findit/waterf...
File appengine/findit/waterfall/schedule_try_job_pipeline.py (left):

https://codereview.chromium.org/2605803002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/schedule_try_job_pipeline.py:55: has_compile_targets,
has_heuristic_results):
I assume we can still keep this function and share it for all 3
schedule_xx_try_job_pipeline ?

[lijeffrey, 2017-01-09 19:04:01]

https://codereview.chromium.org/2605803002/diff/80001/appengine/findit/model/...
File appengine/findit/model/wf_try_job_data.py (right):

https://codereview.chromium.org/2605803002/diff/80001/appengine/findit/model/...
appengine/findit/model/wf_try_job_data.py:31: def master_name(self):
On 2017/01/07 02:39:39, chanli wrote:
> Since both wf_try_job and flake_try_job have master_name, builder_name and
> build_number, can we move these to base_try_job?

master_name and builder_name moved to base class.

https://codereview.chromium.org/2605803002/diff/80001/appengine/findit/model/...
appengine/findit/model/wf_try_job_data.py:31: def master_name(self):
On 2017/01/07 02:39:39, chanli wrote:
> Not sure if we should have similar implementation as flake_try_job_data (move
> the decoding to TryJob model)

Done.

https://codereview.chromium.org/2605803002/diff/80001/appengine/findit/waterf...
File appengine/findit/waterfall/schedule_try_job_pipeline.py (left):

https://codereview.chromium.org/2605803002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/schedule_try_job_pipeline.py:55: has_compile_targets,
has_heuristic_results):
On 2017/01/07 02:39:39, chanli wrote:
> I assume we can still keep this function and share it for all 3
> schedule_xx_try_job_pipeline ?

I think this function is different enough for all 3 we should keep them
separate: for example, all 3 have different types, flake try jobs create a
different entity type and don't have build_number, compile_targets, or
heuristic_results. I think refactoring it into a common base function and 3
separate compile/test/flake functions to handle the type-specific set up may
actually make the code harder to follow.

[chanli, 2017-01-09 19:33:28]

On 2017/01/09 19:04:01, lijeffrey wrote:
>
https://codereview.chromium.org/2605803002/diff/80001/appengine/findit/model/...
> File appengine/findit/model/wf_try_job_data.py (right):
> 
>
https://codereview.chromium.org/2605803002/diff/80001/appengine/findit/model/...
> appengine/findit/model/wf_try_job_data.py:31: def master_name(self):
> On 2017/01/07 02:39:39, chanli wrote:
> > Since both wf_try_job and flake_try_job have master_name, builder_name and
> > build_number, can we move these to base_try_job?
> 
> master_name and builder_name moved to base class.
> 
>
https://codereview.chromium.org/2605803002/diff/80001/appengine/findit/model/...
> appengine/findit/model/wf_try_job_data.py:31: def master_name(self):
> On 2017/01/07 02:39:39, chanli wrote:
> > Not sure if we should have similar implementation as flake_try_job_data
(move
> > the decoding to TryJob model)
> 
> Done.
> 
>
https://codereview.chromium.org/2605803002/diff/80001/appengine/findit/waterf...
> File appengine/findit/waterfall/schedule_try_job_pipeline.py (left):
> 
>
https://codereview.chromium.org/2605803002/diff/80001/appengine/findit/waterf...
> appengine/findit/waterfall/schedule_try_job_pipeline.py:55:
has_compile_targets,
> has_heuristic_results):
> On 2017/01/07 02:39:39, chanli wrote:
> > I assume we can still keep this function and share it for all 3
> > schedule_xx_try_job_pipeline ?
> 
> I think this function is different enough for all 3 we should keep them
> separate: for example, all 3 have different types, flake try jobs create a
> different entity type and don't have build_number, compile_targets, or
> heuristic_results. I think refactoring it into a common base function and 3
> separate compile/test/flake functions to handle the type-specific set up may
> actually make the code harder to follow.

lgtm

[lijeffrey, 2017-01-09 20:06:33]

The CQ bit was checked by lijeffrey@chromium.org

[lijeffrey, 2017-01-09 20:06:34]

The patchset sent to the CQ was uploaded after l-g-t-m from stgao@chromium.org,
chanli@chromium.org
Link to the patchset: https://codereview.chromium.org/2605803002/#ps120001
(title: "Fixing code coverage")

[commit-bot: I haz the power, 2017-01-09 20:06:41]

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, 2017-01-09 20:26:40]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483992393735340,
"parent_rev": "5d82137130f3501da068e1eda027429a3f508a0e", "commit_rev":
"33ffa3ac64e76a71df2f565bd98b144c17bbafd1"}

[commit-bot: I haz the power, 2017-01-09 20:26:43]

Description was changed from

==========
[Findit] Refactoring WfTryJobData into BaseTryJobData, WfTryJobData, and
FlakeTryJobData

In preparation for triggering flake try jobs, the corresponding try job data
entity needs to be refactored to separate WfTryJob from FlakeTryJob

BUG=672724
==========

to

==========
[Findit] Refactoring WfTryJobData into BaseTryJobData, WfTryJobData, and
FlakeTryJobData

In preparation for triggering flake try jobs, the corresponding try job data
entity needs to be refactored to separate WfTryJob from FlakeTryJob

BUG=672724

Review-Url: https://codereview.chromium.org/2605803002
Committed:
https://chromium.googlesource.com/infra/infra/+/33ffa3ac64e76a71df2f565bd98b1...
==========

[commit-bot: I haz the power, 2017-01-09 20:26:44]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/infra/infra/+/33ffa3ac64e76a71df2f565bd98b1...