launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

The proposal to merge lp:~jtv/juju-core/mpv-storage-list into lp:~maas-maintainers/juju-core/maas-provider-skeleton has been updated.

Description changed to:

This was spliced out of a larger ongoing branch which implements file upload & retrieval.  Hence any “clairvoyance” in terms of unneeded (for now!) complexity: getSnapshot() for instance becomes useful in the other branch.  After discussion Julian and I decided not to bother with supporting slashes in filenames for now, but this does test that *if* you have such files, List() will treat them in the documented way.

Speaking of getSnapshot: you may be wondering why I did that.  It's a bit of pioneering in the face of uncertainty.  We haven't really got a handle yet on what needs locking in the provider classes.  Meanwhile, Go's “defer” makes it easy to hold locks for longer than intended — e.g. because you're making an http request in a function that no longer really needs a lock, but still has the lock's release pending until the function returns.  And finally, it's hard to predict at what level in the call tree you need to do your locking.  When you're already holding a lock on an object, sync.Mutex won't grant you a second simultaneous lock on that same object, because the lock is meant to synchronize not just threads but also goroutines which may or may not be executed by the same thread.

And so I sidestepped the question by creating a source of simple, good-enough-for-now consistent snapshots.  You hold your lock very very briefly, and then you go on with an object that at some point may be outdated, but will never be inconsistent.  If you really need an up-to-date version, you'll have to hold a lock while you're working, or if you're an optimist, verify that your snapshot is still current before committing to anything.  But hopefully that won't be needed for the general case.  If your URL for the MAAS API suddenly changed, for instance, failure and retry will probably be about as good as trying to use the latest URL at all times.

Unfortunately there seems to be no good way to know the current state of a Mutex (held or available), or make an attempt to grab it without potentially blocking forever.  So to test that the mutex is available, I just compare it to a freshly-constructed one and that seems to work.  I feel duly guilty about it, but console myself with the thought that if this trick ever becomes inappropriate, the test suite will report a false positive rather than a false negative.  In other words, the compiler or test suite will tell you something's broken.  It won't give you a false impression that all is well.

Jeroen

For more details, see:
https://code.launchpad.net/~jtv/juju-core/mpv-storage-list/+merge/151449
-- 
https://code.launchpad.net/~jtv/juju-core/mpv-storage-list/+merge/151449
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/juju-core/mpv-storage-list into lp:~maas-maintainers/juju-core/maas-provider-skeleton.