[Kde-pim] Akonadi batch notifications review

[Daniel Vrátil]

On Monday 20 of May 2013 20:07:26 laurent Montel wrote:
> Hi,
> On two computers, maildir is blocked after sometime.
> I need to delete my agent_config_akonadi_maildir_resource_0_changes.dat to
> unlock it :(
> .dat file must be broken.

Hi,

I was not able to reproduce this locally. Could you try to provide more data? 
Especially a GDB backtrace of the blocked maildir agent and debug output of 
Akonadi server and the agent.

Thanks,
Dan

> 
> (I just selected some mails that’s all. I didn’t try to move mails or delete
> mails)
> 
> Regards.
> 
> Le samedi 18 mai 2013 19:05:24 Daniel Vrátil a écrit :
> > On Saturday 18 of May 2013 13:12:26 Volker Krause wrote:
> > > Hi,
> > > 
> > > thanks a lot for implementing this! And sorry for taking so long with
> > > the
> > > review.
> > 
> > Hi,
> > 
> > thanks for reviewing the code. I've fixed the issues you mentioned and
> > merged the branches in akonadi, kdepimlibs and kdepim-runtime to master.
> > 
> > Guys, if you see any regressions, please let me know asap :-)
> > 
> > > On Monday 29 April 2013 21:28:41 Daniel Vrátil wrote:
> > <snip>
> > 
> > > > A bit of elaboration on the changes I've done.
> > > > 
> > > > Akonadi Server
> > > > Akonadi Server now uses and emits NotificationMessageV2. Unlike the
> > > > "V1",
> > > > it can represent a single operation on multiple entities. Akonadi
> > > > server
> > > > emits the NotificationMessageV2 via notifyV2() signal, and when
> > > > there's
> > > > at least one subscriber that does not support the new version (support
> > > > for V2 is announced via client capabilities), it splits the V2
> > > > notification into multiple V1 notifications and emits them "the old
> > > > way",
> > > > via notify(). A completely new operation called "ModifyFlags" has been
> > > > added. For this operation, the NotificationMessageV2 has two
> > > > additional
> > > > attributes - addedFlags and removedFlags. When
> > > > Akonadi::AgentBase::ObserverV3 is used itemModified() will no longer
> > > > notify about flag changes (see below).
> > > 
> > > Just some details, the overall approach looks good:
> > > DataStore::setItemFlags(): if the transaction is really needed there
> > > (and
> > > not already done by the caller), use the Transaction RAII class to
> > > ensure
> > > automatic rollback (there might be exceptions in there).
> > 
> > Dropped the transaction completely - it's called from a transaction in
> > Storage::parseStream()
> > 
> > > NotificationMessageV2::List should probably be a QVector, we couldn't
> > > change that in V1 anymore.
> > 
> > Done. Had to add NotificationMessageV2::appendAndCompress() for QList,
> > because in Monitor we work with QQueue, which cannot be converted to
> > non-const reference to QVector.
> > 
> > > NotificationMessageV2::Item should probably be called "Entity" if it
> > > also
> > > can represent Collections.
> > 
> > Done
> > 
> > > > Akonadi::Monitor
> > 
> > <snip>
> > 
> > > > Akonadi::AgentBase::ObserverV3
> > > > A new ObserverV3 has been added, with virtual method for every new
> > > > batch
> > > > operation. Very important thing here is that if you use ObserverV3,
> > > > you
> > > > will never be delivered the non-batch versions of notification from V1
> > > > or
> > > > V2. So even when ObserverV3::itemsRemoved() is not reimplemented in an
> > > > agent/resource, ObserverV2::itemRemoved() will simply not be called.
> > > > This
> > > > is because of design limitation, that would require the Observer
> > > > telling
> > > > back to Monitor to re-queue, split and re-emit the batch notification
> > > > as
> > > > a set of per- item notifications.
> > > 
> > > Yep, keeping backward compatibility there is tricky, but I like your
> > > approach, the ObserverV3 constraints are fine IMHO.
> > > 
> > > We can look into how to simplify all this for KDE5 then, is there a
> > > use-case for keeping the single object signals?
> > 
> > I agree we could merge all Observers to one and drop single-item singals.
> > If users don't want to support batches in their resources, they can just
> > use Q_FOREACH :-)
> > 
> > > One issue in the implementation:
> > > ResourceBase::itemsX(): these skip all changes if any of the items is
> > > invalid (empty RID), that seems a bit extreme, maybe just drop them from
> > > the list instead?
> > 
> > Fixed.
> > 
> > 
> > Cheers,
> > Dan
> > 
> > > > IMAP resource
> > > > IMAP resource has been updated to support itemsMoved and itemsRemoved
> > > > and
> > > > itemsFlagsChanged.
> > > 
> > > Looks good.
> > > 
> > > 
> > > Feel free to merge it, and thanks again for all the work :) If anyone
> > > happens to look for things to work on, we can now optimize many
> > > resources
> > > by making use of the new notifications :)
> > > 
> > > regards,
> > > Volker
-- 
Daniel Vrátil
Associate Software Engineer, KDE Desktop Team
Red Hat, Inc

GPG Key: 0xC59D614F6F4AE348
Fingerprint: 4EC1 86E3 C54E 0B39 5FDD B5FB C59D 614F 6F4A E348
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20130521/b6fa9347/attachment.sig>
-------------- next part --------------
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/