Item-repository allocation/locking refactor
intelfx100 at gmail.com
Wed Dec 19 13:33:56 UTC 2012
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".
> 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.
Please, review the next iteration. It's on the same branch, rebased against
current master (non-fast-forward, of course).
> - 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.
Done using ICore::shutdownCompleted().
> - I dislike that QUuid is used in repositoryPathForSession and
> deleteRepositoryFromDisk. This should imo be an ISession* to make the
> coupling clear.
> 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.
> 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.
Not done: in some situations like the session picker dialog nothing is created
yet, so there is no place to emit signals from. Also, it seems like unneeded
complexity to me: deleteRepositoryFromDisk() is properly guarded, taking
session locks and active session into account.
> - ItemRepoReg::self's comment is odd, remove the "and the only"
> - 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
Done, refactored the whole logic.
> - the public API for ItemRepositoryRegistry::open should take an ISession*,
> otherwise I could open arbitrary locations (which we do not want)
Not done - maybe we'll need to open arbitrary locations someday. If you
insist, I'd better merge the function into constructor as there will be no
possible use-cases for it: why one would need to open a cache for session A
using the directory of session B?
> - while you are at it, move all classes in itemrepository.h into their own
> files and ensure that public API is properly pimpled (esp.
Done. Only ItemRepositoryRegistry is pimpled - other classes are templated.
> - 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?
Created tests for:
- automatic deletion of itemrepository directory of temporary sessions
- manual deletion of itemrepository directory of the running session
The "offline deletion" (delete the directory belonging to a different session)
can't be tested due to impossibility to create more than one instance of Core
from a single launch.
> Cheers, and many thanks for looking into this.
More information about the KDevelop-devel