1571643 - Plug the retriggering into Compare view

Status: RESOLVED FIXED

(Tree Management :: Perfherder, task, P2)

Description

[Ionuț Goldan [:igoldan]]

More details here.

Comment 1

[Alexandru Ionescu (needinfo me) [:alexandrui]]

We need to:
cover the scenario when the user is not logged in
spin the button after it was pressed
show the notification that the jobs were triggered

Comment 2

[Ionuț Goldan [:igoldan]]

At these point we're able to determine the id of the jobs we want to retrigger. Problem is JobModel.retrigger() requires job instances as parameters.

Skimming through the code, I can see here that there's already a job id -> job instance conversion mechanism using Redux.
Am I right about this?

Comment 3

[Sarah Clements [:sclements]]

Cam knows more about that part of the code base but looking at theJobModel.retrigger method why would you need to provide an instance as an arg? It looks like a jobs array would be fine?

Comment 4

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Sarah, we need a button in Compare page (CompareTable) similar to the retrigger button in job view (ActionBar). Here (in ActionBar) there's a context of first selecting and pinning a job before doing any action. So we need to create that context in CompareTable to be able to retrigger a job. igoldan thought about something that could return a job instance using a job id, I thought about a way of selecting the job so then we could pin and retrigger it. Could you or Cam suggest an efficient way of doing that?
Also, could you tell the difference between
PropTypes.object.isRequired
and
PropTypes.object ?

Comment 5

[Sarah Clements [:sclements]]

Alexandru, Cameron would be a better person to ask about that logic in general. But I'm wondering why you would need to pin a job first? I think that is done for Treeherder because it's a requirement for that UI specifically. If you just need to retrigger a job, then I'd think just have a retriggerJob method would be fine. It doesn't look like the JobModel requires usage of member fields/variables so I don't think you'd even need to instantiate it, just use JobModel.retriggerJob. You can see how it's used in that file - looks like an array is passed in for the jobs arg.

Comment 6

[Cameron Dawson [:camd]]

I agree with Sarah. You can see an example of retriggering outside of Treeherder here:
https://github.com/mozilla/treeherder/blob/0ddd5a80e1aa0441199ce1d15da1aeea2a220033/ui/push-health/TestFailure.jsx#L30

Comment 7

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Cam,

If you look at the definition of the TestFailure class you'll see that it inherits a job object. The definition of the CompareTable class I'm working with doesn't get any job instance, I only have the data.results.originalJobIds array that contains some job ids. So I need a function that is returning a job instance based on the job id. It wouldn't really be helpful to get the job in the class definition because this compare page(this link is just an example) contains a table generated by looping through this data array, so each of those retrigger buttons (placed on each row of the # Runs column) depends on every iteration's results.originalJobIds.

Here is a pull request preview of when I worked so far: https://github.com/mozilla/treeherder/compare/master...alexandru-io:retrigger_compare_view?expand=1

Comment 8

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/5259

Comment 9

[Sarah Clements [:sclements]]

Hrmm, looking at the JobModel.retriggerJob again it seems you need push_ids, not job_ids. Cam, can a job be retriggered with a job id?

If you need push_ids then you'll need to modify the /performance/summary endpoint to return that field. Also that whole JobModel class and method is using notify which is something Treeherder uses for all of its notifications. Perfherder uses errorMessages at the top level component, which is the validation higher order component. I think you'll need to extract whats needed from that retrigger method to adapt to Perfherder's requirements (possibly putting some shared logic in one of our ui/helpers files).

Also please do not use jquery as you have here: https://github.com/mozilla/treeherder/compare/master...alexandru-io:retrigger_compare_view?expand=1#diff-645b8f7bcdcc827feda0e8ddd69c6774R38

We have deprecated it's use and setting a spinner or any other className attribute is easy enough to do using react state. There's a loading spinner component you can use in the ui/shared file.

Comment 10

[Cameron Dawson [:camd]]

I need to re-work that models.JobModel to not require the notify and just return whatever is the error. It's "on my list" as one might say.. That being said, it's just a function, so you can pass in a custom function that puts the message anywhere you like.

So, that being said, we could handle this in a couple ways. What we really need from the jobs is the push_id and each job's job_type_name. So, if you wanted to leave the JobModel.retrigger function intact, you could simply call the /jobs/ endpoint with that list of ids and get a list of jobs back and pass those into the retrigger function. We need the push_id for each job, because we have to find the Decision Task for the job in order to retrigger. JobModel.retrigger looks at the list of jobs, extracts the unique push_ids and gets all the decision tasks for them to make this process more efficient.

Otherwise, as Sarah said, you could modify your /performance/summary endpoint to return the push_id for each job (maybe they're all going to be in the same push?). But you'll also need the job_type_name for each job. This is required by Taskcluster and is out of our control. Then you could take your data and create an array of "jobs" that are just { id: 123, job_type_name: "foo job name", push_id: 456 }. Then pass that to JobModel.retrigger and let it parse out what it needs.

Hopefully this makes sense. If not, please ask and I can help further. :)

Comment 11

[Alexandru Irimovici]

(In reply to Cameron Dawson [:camd] from comment #10)

I need to re-work that models.JobModel to not require the notify and just return whatever is the error. It's "on my list" as one might say.. That being said, it's just a function, so you can pass in a custom function that puts the message anywhere you like.

So, that being said, we could handle this in a couple ways. What we really need from the jobs is the push_id and each job's job_type_name. So, if you wanted to leave the JobModel.retrigger function intact, you could simply call the /jobs/ endpoint with that list of ids and get a list of jobs back and pass those into the retrigger function. We need the push_id for each job, because we have to find the Decision Task for the job in order to retrigger. JobModel.retrigger looks at the list of jobs, extracts the unique push_ids and gets all the decision tasks for them to make this process more efficient.

Hi Cameron, I'm going to take over this and I'll use the first approach you suggested. Thanks

Comment 12

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/5304

Comment 13

[Ionuț Goldan [:igoldan]]

We get the following error when building the *.js source code:

ERROR in entrypoint size limit: The following entrypoint(s) combined asset size exceeds the recommended limit (2 MiB). This can impact web performance.

Entrypoints:

  perf (2.01 MiB)

      assets/runtime.e1f76f78.js

      assets/1.4fcf2627.js

      assets/2.090c6af5.css

      assets/2.03bfa618.js

      assets/3.c31181f8.js

      assets/4.4df46e71.js

      assets/perf.5183e939.css

      assets/perf.ed9d3329.js

Comment 14

[Sarah Clements [:sclements]]

You need to temporarily bump up the entrypoint size in the .neutrinorc.js file. Once I've removed angular and deps, the entry point can be lowered to a reasonable limit. Cameron has just bumped it up for one of his pr's, so I'd rebase first and see if that is fine for your purposes.

Comment 15

[Ionuț Goldan [:igoldan]]

This just got merged to master.

Comment 16

[Ionuț Goldan [:igoldan]]

This got deployed to master.