Item-repository allocation/locking refactor

[Ivan Shapovalov]

On 07 December 2012 10:25:06 Ivan Shapovalov wrote:
> On 06 December 2012 23:44:10 Milian Wolff wrote:
> > On Wednesday 05 December 2012 10:11:13 Ivan Shapovalov wrote:
> > > Hi!
> > > 
> > > Following Milian's suggestion on
> > > https://git.reviewboard.kde.org/r/106945/,
> > > I'm putting the work on refactoring the "disk storage allocation" part
> > > of
> > > the item-repositories into a public repo instead of a review-request.
> > > 
> > > The refactoring is just about
> > > - getting rid of locking in the item-repository directories (because we
> > > have rather good per-session locks);
> > > - getting rid of numbered subdirectories inside of per-session cache
> > > directories (because nobody shall use and nobody actually uses multiple
> > > ItemRepositoryRegistries);
> > > - changing the cache path once again
> > > 
> > >   from "~/.cache/kdevduchain/$session"
> > >   to "~/.cache/kdevduchain/$appname-$session";
> > > 
> > > - making ItemRepositoryRegistry a true singleton;
> > > - finally making the cache directories deleted when a session is deleted
> > > from disk.
> > > 
> > > The real reason behind all this was the last point.
> > > 
> > > The branch is called "itemrepository" and is on
> > > "git://github.com/intelfx/kdevplatform.git".
> > 
> > Cool!
> > 
> > Some points I spotted so far - please don't be overwhelmed by that... But
> > I
> > rather want it to merge a nicely designed refactoring which should make
> > this stuff future proof.
> > 
> > - testcore mentioned in DUChain is bad - that should not need any special
> > handling. Put anything you need into ICore, such as an aboutToShutdown
> > signal and connect to it using ICore::self() (which is then a TestCore in
> > a
> > unit test). Actually there is already a aboutToShutdown, which should be
> > used instead of QApplication::aboutToQuit in the DUChain.
> > 
> > - I dislike that QUuid is used in repositoryPathForSession and
> > deleteRepositoryFromDisk. This should imo be an ISession* to make the
> > coupling clear.
> 
> Ok.
> 
> > Also, both of these should not be public API. Rather, you could e.g. add a
> > signal in ISession which notifies about the deletion of the session (from
> > disk) and act accordingly. I'd opt for a aboutToDeleteFromDisk() which
> > gets
> > emitted in Session::deleteFromDisk before the removeDirectory. Then listen
> > to that signal in the registry and act upon it.
> 
> Sounds reasonable - btw, the signal shall be in ISessionController (and not
> in ISession), right?

1) My mistake: SessionController, not ISessionController.
2) Even if it were in SessionController, who'd emit it, say, when session is 
deleted in SessionChooserDialog ("kdevelop -ps")? Nothing is created at this 
point - core does not exist yet.

> 
> > The way it stands now, I could e.g. call "delete" on a session that is
> > currently running in a different session and then when I close that
> > session
> > magically my cache disappears - this is nothing we want. You only ever
> > want
> > to delete your own session's cache.
> 
> No, currently it allows to delete either our session's cache (in which case
> it is performed upon shutdown) or an unlocked/unused session's cache.
> Locked sessions are not touched.
> 
> > - ItemRepoReg::self's comment is odd, remove the "and the only"
> 
> Ok.
> 
> > - in ::open you now do a QDir::count and then check again for a version*
> > file in that dir if no files exists - that makes no sense. Also you should
> > probably use QDirIterator::hasNext instead of counting files if all you
> > need to figure out is whether the dir is empty. imo you should not do such
> > a check at all though - an empty dir should be handled like a dir that has
> > not yet been created
> 
> I haven't actually refactored ::open(), just mechanically changed it... But
> sure, will fix.
> 
> > - the public API for ItemRepositoryRegistry::open should take an
> > ISession*,
> > otherwise I could open arbitrary locations (which we do not want)
> 
> Actually? Maybe it would be useful someday to override cache directory...
> 
> > - while you are at it, move all classes in itemrepository.h into their own
> > files and ensure that public API is properly pimpled (esp.
> > ItemRepositoryRegistry).
> 
> Ok.
> 
> > - I'm missing unit/integration tests for this stuff. Could you maybe try
> > to
> > create some which ensure this new stuff works as it should?
> 
> Sure, I'll do the tests after the actual features and APIs are acked.
> 
> > Cheers, and many thanks for looking into this.
> 
> Thanks for reviewing! :)
> 
> - Ivan