----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118231/#review58317 -----------------------------------------------------------
Overall, the code is getting quite complex, and it's at a state where I would not be comfortable modifying stuff without unit tests. src/pim/agent/collectionindexingjob.h <https://git.reviewboard.kde.org/r/118231/#comment40571> The class name is different than the header name. src/pim/agent/collectionindexingjob.h <https://git.reviewboard.kde.org/r/118231/#comment40574> Whoa. This variable name is very very confusing. It's true, if there are indexed items which are unindexed? :O src/pim/agent/collectionindexingjob.cpp <https://git.reviewboard.kde.org/r/118231/#comment40576> typo src/pim/agent/collectionindexingjob.cpp <https://git.reviewboard.kde.org/r/118231/#comment40572> You can just directly remove the item, and then check if it was successfully removed. src/pim/agent/collectionindexingjob.cpp <https://git.reviewboard.kde.org/r/118231/#comment40573> I'm sorry. I'm a little confused as to how this could ever happen unless Akonadi is messed up and does not reliably send item removed notifications? src/pim/agent/index.h <https://git.reviewboard.kde.org/r/118231/#comment40567> I'm not too happy with this classes name. How about "Indexer" instead? But then it would clash with "AbstractIndexer". src/pim/agent/index.h <https://git.reviewboard.kde.org/r/118231/#comment40569> Perhaps this can be made private? Ditto for some other functions. src/pim/agent/index.cpp <https://git.reviewboard.kde.org/r/118231/#comment40568> Is this required? From a Xapian point of view you're spending an extra amount of time first removing the data and then adding it back again. If you just index the item, Xapian will internally do a diff on the terms that have changed, and then just update those. src/pim/agent/scheduler.h <https://git.reviewboard.kde.org/r/118231/#comment40570> This is confusing. Perhaps some more documentation? Also, couldn't you just directly do a QMap<Akonadi::Collection::ID, QQueue<Akonadi::Item::ID>> I'm not sure what the shared pointer is doing. src/pim/agent/scheduler.cpp <https://git.reviewboard.kde.org/r/118231/#comment40577> Why 100? src/pim/agent/scheduler.cpp <https://git.reviewboard.kde.org/r/118231/#comment40579> This is quite dangerous. If an email is not indexed it results in the entire collection being sync and all a full collection fetch job going on. I'm very much against this. I get enough angry emails about how the baloo indexer is sucking all their cpu. src/pim/agent/scheduler.cpp <https://git.reviewboard.kde.org/r/118231/#comment40578> So, even if someone switched off their system before the initial indexing was done, we mark the initial indexing as completed? src/pim/agent/scheduler.cpp <https://git.reviewboard.kde.org/r/118231/#comment40575> So each time an item is added, you pass the parent collection to the CollectionIndexingJob and there you fetch the entire collection (+statistics) and do a query on Xapian to check how many items are already indexed? Arguably, the xapian query would be quite fast, but still. - Vishesh Handa On May 20, 2014, 10:22 p.m., Christian Mollekopf wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/118231/ > ----------------------------------------------------------- > > (Updated May 20, 2014, 10:22 p.m.) > > > Review request for Baloo and Vishesh Handa. > > > Repository: baloo > > > Description > ------- > > A scheduler for baloo: > * delays the indexing until no new item has been added for at least 5 seconds > to avoid indexing during a collection sync. > * remembers if it failed to index something and triggers recovery path on > next start. > * supports manual triggering of recovery path if required. > > > Diffs > ----- > > src/pim/agent/CMakeLists.txt e917915a3414738595caea5497859ef4810ec44c > src/pim/agent/agent.h 1dbf0fc0a16d0615dbfa4878706359bb687facd0 > src/pim/agent/agent.cpp 8904d49d3579b58b634d2570fbcc8007e5ee41ed > src/pim/agent/collectionindexingjob.h PRE-CREATION > src/pim/agent/collectionindexingjob.cpp PRE-CREATION > src/pim/agent/index.h PRE-CREATION > src/pim/agent/index.cpp PRE-CREATION > src/pim/agent/scheduler.h PRE-CREATION > src/pim/agent/scheduler.cpp PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/118231/diff/ > > > Testing > ------- > > I'm running it for a while, and it reduced the stress that baloo imposed on > my system and all my mails are indexed since I'm using it (wasn't the case > before). > > > Thanks, > > Christian Mollekopf > >
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
