Merge lp://qastaging/~jelmer/launchpad/upgrade-stderr into lp://qastaging/launchpad

merge approve
vote approve

I wonder if the automatic upgrades will be safe. Perhaps you should check
with Tim or guard it with a flag, if they're available there.
On Jul 21, 2011 3:30 AM, "Jelmer Vernooij" <email address hidden> wrote:
> Jelmer Vernooij has proposed merging lp:~jelmer/launchpad/upgrade-stderr
into lp:launchpad.
>
> Requested reviews:
> Launchpad code reviewers (launchpad-reviewers)
> Related bugs:
> Bug #808035 in Launchpad itself: "AttributeError: 'NoneType' object has no
attribute 'write' during git import"
> https://bugs.launchpad.net/launchpad/+bug/808035
>
> For more details, see:
> https://code.launchpad.net/~jelmer/launchpad/upgrade-stderr/+merge/68579
>
> Fix processing of code imports that are in an old bzr format:
>
> 1) Implement show_user_warning in the custom LoggingUIFactory used in the
code import
> workers. This fixes the immediate issue that causes the traceback,
> and will prevent issues in the future if parts of bazaar print user
warnings.
>
> 2) Reintroduce the automatic upgrading of code imports, and fix the
related test.
> Previously the test would compare the formats of the bzr dirs, both of
which
> would be "Bazaar-NG meta directory, format 1". The test now compares the
> repository formats, which are different.
> --
> https://code.launchpad.net/~jelmer/launchpad/upgrade-stderr/+merge/68579
> You are subscribed to branch lp:launchpad.

I'm going to second pinging Tim to make sure this is safe; I see that the associated bug in his XXX is fix released (though its flags seem off), but I'd like to make sure we can safely throw the automatic upgrade switch before approving.

Hi Tim,

Since you last touched the auto upgrade code in Launchpad before me, can you perhaps review this branch? We've had some fall out from this earlier, so would like to be extra sure it's doing the right thing.

> I'm going to second pinging Tim to make sure this is safe; I see that the
> associated bug in his XXX is fix released (though its flags seem off), but I'd
> like to make sure we can safely throw the automatic upgrade switch before
> approving.
Thanks for the review.

The XXX bug was fixed earlier but then rolled back because of an incident ( https://wiki.canonical.com/IncidentReports/2011-06-17-LP-codeimport-upgrade-breakage)

The primary reason that we disabled the automatic upgrade was that there are hundreds, maybe thousands of code imports in the old format. Also upgrading to packs was a slow process. We only had so code import workers, and we didn't want to clog them all up doing upgrades.

Perhaps the best solution is to work out how to limit the number of workers on any given slave are doing upgrades.

Perhaps we should land just the stderr fix, and then do the upgrades
asynchronously from imports?

Hi Tim!

Thanks for looking at this.

> The primary reason that we disabled the automatic upgrade was that there are
> hundreds, maybe thousands of code imports in the old format. Also upgrading
> to packs was a slow process. We only had so code import workers, and we
> didn't want to clog them all up doing upgrades.
>
> Perhaps the best solution is to work out how to limit the number of workers on
> any given slave are doing upgrades.
I don't think this problem is as bad as it used to be. The upgrade code in Bazaar has improved really significantly since the auto-upgrades were disabled in Launchpad, and is now quicker than the foreign plugins.

All of the branches that were enabled and in the older format have been marked as failing because of the regression (introduced around July 1) that's fixed by this MP. According to the FailingCodeImportsMetrics graph, that appears to be between 65 branches and 100 branches.