hard to get from mp to per-revision diffs

Bug #813349 reported by Martin Pool
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Launchpad itself
Triaged
High
Unassigned

Bug Description

It would be more useful if you could review individual mp component diffs inline.

This is now done, but it does not work for private branches (bug 806713).

Related branches

Revision history for this message
Martin Pool (mbp) wrote :

poolie: if it deploys i will have poke at it for you
spiv: Thanks! The main thing for QA I think is to see that without the feature flag it doesn't break the branch merge proposal page (I'm sure it'll be fine),
spiv: and then if the feature flag is set (maybe just for canonifolk or beta-testers?) that the expanders exist and work on that page.

Revision history for this message
Launchpad QA Bot (lpqabot) wrote :
tags: added: qa-needstesting
Changed in launchpad:
status: In Progress → Fix Committed
Revision history for this message
Martin Pool (mbp) wrote :

hloeung is helping me test this on qas.

It's a little complicated because the branch scanner and mp cron jobs do not regularly run and the mps from production are not available.

I created https://code.qastaging.launchpad.net/~mbp/bzr/doc-2.4 and a mp off it and I'm waiting to get the mp actually fully populated.

Revision history for this message
Martin Pool (mbp) wrote :

With the default (flag off) things look ok.

Haw added <https://code.qastaging.launchpad.net/+feature-changelog>

code.ajax_revision_diffs.enabled team:launchpad 1 on

And I see that in the footer comment, but I don't see any expanders yet.

Revision history for this message
Martin Pool (mbp) wrote :

OK, in <https://code.qastaging.launchpad.net/~mbp/bzr/doc-2.4/+merge/65290> I can see the diff appearing on the incremental revisions.

We ought to also add this feature on the initial mp revisions, and on the branch itself, but that can be done separately.

Also this makes it stick out to me how weirdly different the inital mp revisions are from those added after it's created.

tags: added: code-review javascript qa-ok ui
removed: qa-needstesting
Martin Pool (mbp)
description: updated
Revision history for this message
Andrew Bennetts (spiv) wrote :

Thanks for QAing that! Looking forward to seeing this on production.

Revision history for this message
Martin Pool (mbp) wrote : Re: [Bug 813349] Re: hard to get from mp to per-revision diffs

This is now live and just waiting for the flag change to be ok'd to
turn it on for ~launchpad for the initial closed beta.

Revision history for this message
William Grant (wgrant) wrote :

It's now switched on and working for ~launchpad.

Revision history for this message
Andrew Bennetts (spiv) wrote :

So grepping the OOPS summaries since this was switched on I see only 1 OOPS that might be related to this, OOPS-2031CO104, and it's more likely that's just an IE6 glitch that we don't care about. (I looked for OOPSes involving /diff/ in their URL, as well as skimming reports matching OOPS-.*CB.)

So it seems this isn't causing any errors. Time to give this wider exposure? Or is there something else that should be done first?

Revision history for this message
Martin Pool (mbp) wrote :

there are at least two follow on bugs, though personally I don't think they need to block rollout to at least ~launchpad-beta:

bug 814696
bug 814697

Revision history for this message
Robert Collins (lifeless) wrote :

The bug about it being very slow on LP branches is particularly
concerning to me - there is another bug that loggerhead does not have
a timeout mechanism; I think if we had one - and we will be adding one
- those requests would be timing out consistently, and its the absence
of that mechanism that means you're not seeing oopses.

I suggest looking at the service times in the apache log / loggerhead
logs instead - anything over 1 second is undesirable, over 5 seconds
is very worrying and 9 seconds would be a critical today, if we had
timeouts wired up for loggerhead.

Revision history for this message
William Grant (wgrant) wrote :

For LP branches, the first request always times out, but after that it works fine.

William Grant (wgrant)
Changed in launchpad:
status: Fix Committed → In Progress
Revision history for this message
Martin Pool (mbp) wrote :

bug 814696 and bug 814697 are now done, though bug 904070 (about handling private branches) is still open.

Martin Pool (mbp)
Changed in launchpad:
assignee: Andrew Bennetts (spiv) → Martin Pool (mbp)
Revision history for this message
Martin Pool (mbp) wrote :

The current state of this is that the code is landed and live, but only for ~launchpad, and it does not work on private branches (bug 904070.) I would like to enable it generally, and leave it just not working for private branches, since they are a fairly minority case, the failure is fairly clean and having it on for most branches is an improvement in general utility.

Changed in launchpad:
assignee: Martin Pool (mbp) → nobody
status: In Progress → Triaged
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.