[MERGE] repository write groups

Wed Aug 1 01:37:06 BST 2007

On Tue, 2007-07-31 at 10:17 -0500, Martin Pool wrote:
> On 7/30/07, Robert Collins <robertc at robertcollins.net> wrote:
> 
> I'm glad you think they're good questions.  Getting this right, I think,
> is important to being able to keep repository-using code clean while
> varying the repository.  I don't mind putting in write groups without
> getting all the way there, so please resend the patch with the docs, and
> we can talk maybe a bit more.

Ok, patch attached. I've added docs, made all the tree.lock_write calls
use try:finally to ensure unlocks occur appropriately, and done the
grammar and spell checking you covered earlier.

> > On Mon, 2007-07-30 at 23:11 -0500, Martin Pool wrote:
> > > On 7/30/07, Robert Collins <robertc at robertcollins.net> wrote:
> > > > On Mon, 2007-07-30 at 22:16 -0500, Martin Pool wrote:
> > > No, that wasn't what I meant.  What I meant was that it would implicitly
> > > take a lock if _and when_ necessary.  For your example, it's during
> > > finalization.  For another it might be the whole time, or not at all.
> > >
> > > With this change, the caller doesn't know when the repository should be
> > > locked.
> >
> > I disagree. The caller should lock around the period they want
> > consistent views of data, just as currently.
> 
> As you say, the names are causing confusion.  "Lock" means "exclude other
> people from writing to this": that's what it means in general, and that's
> what the lock elements in our control files do.  My point is that we're
> changing it so that the caller no longer directly controls that or even
> sees when it's happening.  Instead, they say when they want to insert data
> or to have a consistent view or whatever, and the repository decides if
> and when it should lock.  No?

"Modern" repositories have this flexability, our own older ones, and
ones we want to interface to, do not have this flexability. I feel like
you are saying that all repositories can do this, and thats not true.
For repositories that use OS locks, the lock_write call is semantically
very important, because upgradable OS locks are not pervasively
available.

> > >   It is kind of an inversion or displacement of control, which is a
> > > good thing because it allows more room for variation by the repo.  But if
> > > the caller no longer controls when the repository is locked, why are they
> > > calling lock_write?
> >
> > I think our method names may be adding confusion, which is a shame,
> > because its less clear. But we've got I think reasonable solid concepts
> > in play in the code base.
> >
> > Callers call lock_write to get:
> >  - the ability to insert data (if you can't lock-write, you cannot
> > insert)
> >  - a consistent view on the data - in principal data cannot be *removed*
> > while anyone has a write lock, regardless of the repository format - and
> > new data inserted by other writers is ignored. In practice until we have
> > a gc command implemented we can't be at all sure we have this nailed. ce
> > la vie.
> 
> On the first: you can't write til you call start_write_group.  So
> lock_write just gives you permission to call that function, which seems a
> bit... bureaucratic.

For the repository format I'm working on it is bureaucratic. For others,
much less so. I don't feel any API pressure to to make it harder for the
others.

> The second would be fine, except that it's not actually true. :-)  If we
> wanted to guard against data being removed while it's in use, we would
> presumably want that when reading as well as when writing, so would need
> to write a lock even when reading. 

Uhm. We've discussed this but perhaps we should write it down. I see
handling data rearrangement as a client side thing. Consider 'pack' as a
more common operation than gc. Basically the client should get an error
they can use to infer a repository changing operation has occured on,
then follow that up by re-reading the list of available things in the
repository. Clearly this needs repository design to achieve. An example:
- pack based repository, unique names for packs
- 'bzr pack' will remove some pack files 'atomically'. That is it will
  - generate new packs
  - write a new list-of-packs
  - move obsoleted packs away
- a client that gets a file not found when reading from a pack it 
  'knows' is there, should:
  - re-read the list of packs
  - re-attempt the high level operation that was in progress

Getting from 'pack' to 'gc' is relatively easy given this approach.
Simply add a token that represents when 'gc' was last done. Define 'gc'
as always writing new packs and renaming old ones - just like 'pack'.
Then a client suffering a file not found error should:
 - re-read the last-gc token
   - if that has changed, error up to the caller, as the consistent 
     view of data contract has been broken (as has data consistency 
     perhaps.
 - else, treat this as a pack occurence.

>  To me this says more about the
> conceptual difficulties of doing gc-in-place with non-locking readers.
> For newer formats, we don't want to hold a physical lock for the whole
> time, so we can't use it to exclude gc.

Right, see above.

> > > One reason might be that they want to rely on keeping some things in
> > > memory so that they can more cheaply do the hooks and update the wt after
> > > the write group finishes.  Maybe they should do lock_read and do the write
> > > group inside that?
> >
> > That requires upgradable locks in some formats, which will be tricky to
> > do for some underlying formats. The big advantage of keeping our current
> > interface and not shuffling things around is that it plays nice with
> > everything from our old all-in-one formats up to the latest stuff I'm
> > working on, and with svn/git/hg as well.
> 
> The weave formats where we have a read lock are now pretty old, and we
> could deprecate writing to them...

My point with them was not that they are something we internally care
about, but that they represent a class of ForeignRepository which I
think is worth supporting in terms of API /design/.

> > I think that the direction and questions you are asking are good ones,
> > but that it would be very easy to define how it should work for an ideal
> > pack based repository, yet make it much harder to thunk well for older
> > formats or some foreign formats. To that end I propose doing nothing in
> > the short term and keeping with:
> > - lock_read to get a cache context
> > - lock_write to get a cache context and the ability to call
> >   start_write_group
> > - start_write_group et al are essentially no-ops for repositories where
> >   lock_write takes out a mutex, and core functionality for ones where
> >   actual transactional functionality can take place.
> >
> > Which is to say, that callers using large scale operations like
> > 'commit', 'merge' etc make no changes, callers doing fine grained things
> > like 'add_inventory' will need to be changed to use write groups, or
> > better yet the Builder style interface for adding commits.
> 
> It seems like for new format we really just want two operations the client
> can ask for: "start caching/discard cache" and "start writing/finish
> writing".

uhm. I guess. I don't have a strong view here; I think what we have
works well, the patches are done modulo anything that isn't properly
interface tested.

> I think the main concrete change I want is that start_write_group should
> not insist on the caller previously taking a lock.  Then at least we can
> move away from having the callers know about locking.  It can implicitly
> take a write lock if the format is one which needs it.  I just don't think
> it should be part of the start_write_group contract.

I think that this implicit lock taking concept is ugly. It makes the
owner of the lock unclear, and its why I didn't just reuse
lock_write/unlock and add 'abort_lock'.

I don't agree that moving callers away from knowing about the functions
called 'lock_write' and 'unlock' is a good thing. We may be back to
method names however.

-Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: repository-write-groups.patch
Type: text/x-patch
Size: 55763 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070801/a897e819/attachment-0001.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070801/a897e819/attachment-0001.pgp 