Closed Bug 1344432 Opened 7 years ago Closed 6 years ago

The UX for landing multiple interdependent bugs via mozreview/autoland is very poor

Categories

(Conduit Graveyard :: Transplant, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: bzbarsky, Unassigned)

References

Details

What I was trying to do today: land patches for 4 bugs, which had to be landed in a particular order.  This is a perfectly normal workflow for me, for what it's worth, when working on something big as opposed to minor bugfixing.

How I would do this on inbound: pull inbound, rebase patches, push to inbound.  Once I'm done rebasing (which in this case was a trivial rebase anyway), total time is 20-30 seconds max.  All the work is done at my terminal prompt.

How this actually worked with mozreview/autoland: pull m-c, rebase patches.  Starting at that point, here is my actual experience today:

1)  Push the patches to mozreview.  This takes a minimum of 4 minutes, due to bug 1343902.  In practice it takes longer, because you have to look up the changeset ids and pass the right -c arguments.  Parts of this can be parallelized with later steps, though.  I didn't parallelize this as well as I could have, I think.  Not least because I have not yet turned on autopublish yet, because I'm not confident enough that mozreview will do what I expect with my revsets.  So this part requires constant babysitting.

2)  Load the first patch in mozreview.  Request autoland.

3)  Wait for it to make it to autoland, because I don't know whether it will merge successfully or not, because I didn't rebase it on autoland (due to bug 1340653).  There's no reasonable notification on the mozreview page when this happens.  There is email notification via the bug comment, or I can keep reloading treeherder or pushlog for autoland or the bug itself.  Either way, it's pretty active waiting, and takes a few minutes.

4)  Request autoland on the second patch.  This can't be done until step 3 is done, because if the first patch fails rebase, this patch must not land (it will fail rebase if I'm lucky, but in my case it would have just turned the tree red).  Again, wait several minutes.

5)  Request autoland on the third patch.  This can't be done until step 4 is done, for the same reasons.  This failed to rebase on autoland, though it took the usual multiple minutes to tell me so.

6)  Pull autoland after all, which I had been trying to avoid, because sometimes autoland has merge changesets that are not on m-c yet (e.g. if m-c has merged to autoland but not vice versa), and if that happens then pushing to mozreview is disallowed.

7)  Rebase on top of autoland.  It was a trivial rebase that hg qpush handled automatically, giving a warning about a bit of fuzz being ignored.

8)  Push the third patch to mozreview again.

9)  Once that's done push the fourth patch to mozreview again.

10) While #9 is running, if I'm being smart, request autoland on the third patch.  Wait for that to come through.

11) Request autoland on the fourth patch.  Wait for it to come through.

Total time: about 25 minutes, almost all of it active time.  I had to split my attention constantly between three things: my terminal, for pushes, the mozreview pages, and one of the four ways of watching autoland I mentioned in item 3.  There were tons of failure-prone steps here: selecting autolanding on the wrong mozreview page (which in fact happened to me), uploading the wrong things to mozreview, etc.  It would have been _very_ easy for me to accidentally land something that did not match the changesets in my tree, and not doing that required a good amount of concentration and double-checking.

I think with practice and familiarity and better parallelization of my workflow I could cut down the time to about 15 minutes in this case.  But it would be a _very_ focused 15 minutes.  Under no circumstances could I cut the time below 6 minutes: if you followed along, I had to do 6 pushes to mozreview, and each one takes at least a minute.

None of this compares particularly favorably with the workflow I experience on inbound.

And note that I got lucky.  After step 6, I might have been unable to perform steps 8 at all (again, see bug 1340653).
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #0)
> What I was trying to do today: land patches for 4 bugs, which had to be
> landed in a particular order.  This is a perfectly normal workflow for me,
> for what it's worth, when working on something big as opposed to minor
> bugfixing.

I think the failure here comes down to MozReview assuming that dependent changes are developed as part of a single series (which due to current restrictions means a single bug) rather than multiple dependent commits spread around several series. While I think proper dependency management would be helpful (although I believe should be automatic based on the DAG) MozReview was built assuming the series will be used for this dependency management.


Keeping your changes, which must land together in a specific order, all tied to a single MozReview review series will make things much smoother. If they are fixing several bugs but it's important they be developed this way, filing a single bug to hold the code and using bugzilla dependencies could help manage that?

 
> 1)  Push the patches to mozreview.  This takes a minimum of 4 minutes, due
> to bug 1343902.  In practice it takes longer, because you have to look up
> the changeset ids and pass the right -c arguments.

Again, the pain here comes from an assumption MozReview is making about the common workflow. If each cohesive set of commits is kept as its own DAG head, updating to that head and doing an "hg push review" just does the right thing (by default it will take the current checked out commit, and all ancestors up to the first non draft phase commit).

When you manage the commits differently MozReview can't guess what you want, so you have to specify the ids. Admittedly the '-c' vs '-r' arguments UX is pretty shitty, but that's a result of us wanting the default case to just work with no arguments, if you're using head based development.


> I think with practice and familiarity and better parallelization of my
> workflow I could cut down the time to about 15 minutes in this case.  But it
> would be a _very_ focused 15 minutes.  Under no circumstances could I cut
> the time below 6 minutes: if you followed along, I had to do 6 pushes to
> mozreview, and each one takes at least a minute.

Keeping all the related changes to a single series would require a single push, 1 minute.

I don't have a great answer for making your specific workflow perform well at this time, hopefully the workflow adjustments I've suggested are viable for you. Going forward there is work being done to improve the autoland experience, both at push from developer machine time, and landing request time. Hopefully this will make things smoother when it is complete.
> I think the failure here comes down to MozReview assuming that 
> dependent changes are developed as part of a single series

Yeah, that's not how it works, sadly.  When working on something significant that can be broken up into coherent separately-landable things that improve the state of the world, I will do just that, so things can land as they are finished as opposed to having to wait months until I get to the end of the task.

> Keeping your changes, which must land together in a specific order

Note that I may not even know the order up front.  In this case I did, but it's also common for me to have several conflicting changes, which I will then land in whatever order they get reviewed in (resolving the conflicts locally).  But at that point I need to land them in that order, because they are conflicting!

I've brought this up repeatedly in conversations about mozreview and people keep pretending like this problem doesn't exist, which is unfortunate.

> filing a single bug to hold the code 

That would be incredibly unwieldy for me, hellish for reviewers, hellish for anyone looking at blame later, super-bad for sheriffs who might need to back out pieces, horrible for release drivers who need to figure out backports, etc.

Basically, any time a bug has some changes land and others not and is left in a bizarre "is it landed?" state, that's horrible for all our normal workflow stuff.  I am NOT going to impose that cost on everyone who needs to deal with my commits.

> Again, the pain here comes from an assumption MozReview is making about the common workflow

This assumption is just totally wrong for a fair number of people.  Again, this is hardly the first time that's been said.

> just does the right thing

No, because of the need to possibly reorder commits depending on the order reviews come in, and because of the assumption that each review series is a single bug.

> if you're using head based development

I do that too, obviously.  About half my fixes can be done that way.  The other half can't.

> hopefully the workflow adjustments I've suggested are viable for you

Absolutely not, see above.  The only conclusion I can draw from the above is that we work on completely different areas of code with completely different commit parallelism levels.  :(  The workflow you suggest would not work at all for the sort of things I have to do all the time, unless I just pushed all the pain off onto my reviewers and the sheriffs and release drivers.  Which I'm not willing to do.

Again, this is hardly news; it's literally been years since these issues were brought up.  :(

That said, I have specific suggestions for making this better (some mutually exclusive):

1)  Make pushes to mozreview perform no worse than pushes to inbound.
2)  Make a push of an entire branch with commits for multiple bugs automatically put all the changesets in
    the right places, instead of assuming that each push corresponds to a single bug.
3)  If #2 is hard for initial review creation, at least make it work for _updates_ to existing reviews.
4)  Have a command-line UI for requesting autoland.  Ideally on an entire branch at once.
5)  Improve the performance of requesting autoland and knowing whether you landed.
    The current multi-minute lag is insane, compared to the time it takes to do a push to inbound.
6)  If #4 is not an option, at least show the autoland status at the place where it was requested, in an
    expeditious way, instead of forcing even more context switches.

Obviously some clear way of saying "these things need to land in a certain order" and then having it Just Happen might be helpful too, and as you note the DAG _already_ encodes that information....  So we might be able to do something there, but that sounds more complicated/architectural than some of the suggestions above.
6)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #2)
> 4)  Have a command-line UI for requesting autoland.  Ideally on an entire
> branch at once.

I recently filed bug 1341778 for this.
Product: MozReview → Conduit
See Also: → 1457525
We won't be fixing MozReview here and Lando has a completely separate set of assumptions so I'm WONTFIXING this. I've called this bug out in Lando's series support, Bug 1457525, so that we make sure to consider any relevant points it brought up.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Product: Conduit → Conduit Graveyard
You need to log in before you can comment on or make changes to this bug.