Item-repository allocation/locking refactor

[Ivan Shapovalov]

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?

> 
> 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