Merge lp:~jameinel/bzr/2.0.4-autopack-rename-507557 into lp:bzr/2.0

I'm pretty happy with the result, but I'm not 100% sure this is a small enough change to go into 2.0.*

Anyway, this changes a few bits of the autopacking code.

1) When renaming a file into 'obsolete_packs', if we get an error, just continue on. We will generate a warning, but it shouldn't interrupt the process (it was already removed from the list of referenced files.)

2) When deleting files from 'obsolete_packs' don't delete files that we currently have scheduled for obsoletion. "If I'm scheduling it 'right now' then probably so did someone else, and they beat me to it."

3) For files found in (2) don't try to rename them (since we know they are already in obsolete_packs).

4) Go one step further. If a file we are about to obsolete has already been removed from 'pack-names', then we can assume that someone else has already scheduled its obsoletion and will be doing the rename instead of us.

This should fix most of the problems in bug #507557. I don't know if it also solves some of the other autopack concurrency issues, but it should at least help with that one.

 review: +1 based on your description alone.

-Rob

The fix is clearly demonstrated by the associated tests which makes
it a good candidate even for 2.0.5.

We may want some staging period so I think release *candidate* is good for that.

(This isn't a review, just some comments from an interested third party...)
* I've done some user-level testing with your fixes and they do appear to resolve the original bug. When stress-testing Bazaar my commits now all succeed, aside from the occasional instance of bug 507553 ;)

* I would suggest making the warnings into mutters simply because (a) they're not something the user should be alarmed about, and (b) when you get them it can be quite spammy (e.g. 6 files per revision = 54 warnings in the worst case). However, see next point.

* I noticed that groupcompress_repo.py already contains a copy of _execute_pack_operations() so you'll probably want to apply your changes there as well (or do some refactoring to eliminate the copy - it's only a couple of lines different). I think this is why I was seeing so many warnings when running my stress test with a 2a repo - it wasn't actually exercising most of your new code.

* I manually applied the above changes and repeated the stress test, and didn't get a warning at all. However, it did appear to be leaving around some pack files in the "packs" directory. I suspect this is because the single-revision pack that's created during a commit just before an autopack never gets added to pack-names. So the "if o.name not in already_obsolete and o.name in orig_disk_names" test fails.

* You've probably already fixed it locally, but I noticed there's still some debugging code in reload_pack_names() ;)

As a customer it would be nice to have this in 2.0.x but since 2.1 is so close I wouldn't be so worried about it.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Gareth White wrote:
> (This isn't a review, just some comments from an interested third party...)
> * I've done some user-level testing with your fixes and they do appear to resolve the original bug. When stress-testing Bazaar my commits now all succeed, aside from the occasional instance of bug 507553 ;)
>
> * I would suggest making the warnings into mutters simply because (a) they're not something the user should be alarmed about, and (b) when you get them it can be quite spammy (e.g. 6 files per revision = 54 warnings in the worst case). However, see next point.
>
> * I noticed that groupcompress_repo.py already contains a copy of _execute_pack_operations() so you'll probably want to apply your changes there as well (or do some refactoring to eliminate the copy - it's only a couple of lines different). I think this is why I was seeing so many warnings when running my stress test with a 2a repo - it wasn't actually exercising most of your new code.

Nice catch.

>
> * I manually applied the above changes and repeated the stress test, and didn't get a warning at all. However, it did appear to be leaving around some pack files in the "packs" directory. I suspect this is because the single-revision pack that's created during a commit just before an autopack never gets added to pack-names. So the "if o.name not in already_obsolete and o.name in orig_disk_names" test fails.

I'm a little concerned about this one. Partially because my direct
testing didn't show this behavior. Leaving around an extra pack file on
every autopack would be bad. Or does this only happen if there is
contention?

>
> * You've probably already fixed it locally, but I noticed there's still some debugging code in reload_pack_names() ;)

Removed. I'm surprised Vila didn't catch it, he's usually pretty good
about spotting my 'pdb' lines. :)

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktYiUIACgkQJdeBCYSNAAMuGgCeOh/C7TX9JYnBA+i8hPdteGnw
3iMAoNn6O9sZu77LlxEDqfHfw6dOuGD1
=haMF
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

>> * I manually applied the above changes and repeated the stress test, and didn't get a warning at all. However, it did appear to be leaving around some pack files in the "packs" directory. I suspect this is because the single-revision pack that's created during a commit just before an autopack never gets added to pack-names. So the "if o.name not in already_obsolete and o.name in orig_disk_names" test fails.
>
> I'm a little concerned about this one. Partially because my direct
> testing didn't show this behavior. Leaving around an extra pack file on
> every autopack would be bad. Or does this only happen if there is
> contention?
>

I've confirmed this (with tests that fail), and I'm in the process of
adding a localized test for this behavior. There have been enough rough
spots that I'll probably just land this for 2.1.0rc1 to start with, and
then keep the patch around for future 2.0.5 once we've had some time
with it.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktYlaMACgkQJdeBCYSNAAMsoQCcDUvjqgpiCPgLaJnzRSvH42zj
7JYAoLuasGQV3kleC5jAT4QhpscOFU64
=PrPm
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John A Meinel wrote:
>
>>> * I manually applied the above changes and repeated the stress test, and didn't get a warning at all. However, it did appear to be leaving around some pack files in the "packs" directory. I suspect this is because the single-revision pack that's created during a commit just before an autopack never gets added to pack-names. So the "if o.name not in already_obsolete and o.name in orig_disk_names" test fails.
>> I'm a little concerned about this one. Partially because my direct
>> testing didn't show this behavior. Leaving around an extra pack file on
>> every autopack would be bad. Or does this only happen if there is
>> contention?
>
>
> I've confirmed this (with tests that fail), and I'm in the process of
> adding a localized test for this behavior. There have been enough rough
> spots that I'll probably just land this for 2.1.0rc1 to start with, and
> then keep the patch around for future 2.0.5 once we've had some time
> with it.
>
> John
> =:->

Here is a possible fix. I don't really like the "isinstance()" check,
but it does fit. (If a 'pack' is a new then it isn't going to be in the
pack-names file, etc.)

The only other problem is that I think a pack stays "NewPack" between
multiple calls to .commit_write_group(), rather than being converted to
ExistingPack after commit_write_group finishes. (It could be done in
synchronize_pack_names_from_disk_nodes, I think.)

I'm tempted to just get rid of that check, and go ahead and let both
clients try to rename all obsolete packs (except for ones it has already
found in the target). And then downgrade the 'warning' to 'mutter'.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktYqE8ACgkQJdeBCYSNAAMEfACfX3qLSvUK6XS880U8b12NUUr5
PnoAoIRfPd52mUOK3mHB3id4FH8MoFim
=opCG
-----END PGP SIGNATURE-----

On Thu, 2010-01-21 at 19:18 +0000, John A Meinel wrote:
>
>
> I'm tempted to just get rid of that check, and go ahead and let both
> clients try to rename all obsolete packs (except for ones it has
> already
> found in the target). And then downgrade the 'warning' to 'mutter'.

+1 on this.

-Rob

Is this fully approved now (given Rob's vote) or do you need a review still?

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ian Clatworthy wrote:
> Is this fully approved now (given Rob's vote) or do you need a review still?

I think this is fully approved (but not marked as such?) I landed it in
2.1 rather than trying to squeeze it into the 2.0.4 series. (There was a
fair number of bumps to this, and I didn't want to push it just before
release.)

It has gotten a bit of a back burner, but I'm not sure right now whether
I want to push for 2.0.5, or just forget it and just have it be in 2.1.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktio84ACgkQJdeBCYSNAAPC9gCfRY+62xkMrE8ryweD/5noc74G
VPEAn2miqkg2rmgok8AsCr3H3b6yEK7p
=Y0r5
-----END PGP SIGNATURE-----

> It has gotten a bit of a back burner, but I'm not sure right now whether
> I want to push for 2.0.5, or just forget it and just have it be in 2.1.

The linked bug has Low priority (fwiw) and Gareth seems ok with it being in 2.1 only. At this point, I'd be tempted to leave it out of the 2.0 series.

John, did you land this patch into 2.1 ? 2.0 ? Dev ?

I'd be inclined to land it into 2.1 only (and to dev from there)
but since you're the RM, it's up to you.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> John, did you land this patch into 2.1 ? 2.0 ? Dev ?
>
> I'd be inclined to land it into 2.1 only (and to dev from there)
> but since you're the RM, it's up to you.

It has landed in 2.1 (a while ago), but since it was originally proposed
for 2.0 it stays around until we land it there. We could mark it as
merged if you find it is in your way, though since we are considering it
for a 2.0.5 inclusion, I didn't want it completely gone.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktxkxsACgkQJdeBCYSNAAOosgCfVPbXNwrkGj2Aocr+HZd8Xnck
d1sAoJ3TSLD24X3SMCHXkksA/SPM87bu
=xXYZ
-----END PGP SIGNATURE-----

John, I think you should either land this, or supersede it if we actually don't care about putting it into 2.0. Given as Ian says the bug is assessed as low and the patch is not trivial, I would probably lean towards not landing it, but either is fine.

I don't want us getting to tolerate a long review queue because other stuff will get lost.

Marking merged since 2.1 is out.
I think we should focus on pushing 2.1 everywhere instead of doing a 2.0.5.

On Mon, 2010-03-01 at 08:04 +0000, Vincent Ladeuil wrote:
> Marking merged since 2.1 is out.
> I think we should focus on pushing 2.1 everywhere instead of doing a 2.0.5.

We're very unlikely to get 2.1 into karmic/hardy etc. Just saying :)

-Rob

On 1 March 2010 19:24, Robert Collins <email address hidden> wrote:
> On Mon, 2010-03-01 at 08:04 +0000, Vincent Ladeuil wrote:
>> Marking merged since 2.1 is out.
>> I think we should focus on pushing 2.1 everywhere instead of doing a 2.0.5.
>
> We're very unlikely to get 2.1 into karmic/hardy etc. Just saying :)

Yes, 2.0 is still very much alive, and should get all patches that
make sense to fix there. However, if the bug is not severe or the fix
is not trivial, it's ok to leave it out.

Generally if the same patch will apply to both 2.1 and 2.0 it should
be merged to 2.0. 2.0 doesn't have a higher bar than 2.1.

--
Martin <http://launchpad.net/~mbp/>

Yeah, digging while you were commenting :) I realized we already have quite a bit of mess between the various patches already landed in the 2.0 and 2.1 branches, I'm cleaning up and will land this patch in the 2.0 branch before merging into 2.1 and then into bzr.dev.