[Kde-pim] Akonadi batch notifications review

[Volker Krause]

Hi,

thanks a lot for implementing this! And sorry for taking so long with the 
review.

On Monday 29 April 2013 21:28:41 Daniel Vrátil wrote:
> Heya,
> 
> back in Berlin I talked with Volker about implementing batch notifications
> in Akonadi. The main idea is that some resources might perform better when
> they are given the entire batch of items instead of serving the items one
> by one. Good example is the IMAP resource. Instead of having to SELECT
> folder and STORE flags for every item separately, it would be better if it
> could SELECT the folder once and then STORE the new flags for the whole set
> of items - making "Mark All As Read" a single operation (also making it
> faster and saving potentially a lot of bandwidth).
> 
> The code is available in batch-notifications branch in Akonadi and akonadi-
> batch-notifications branch in kdepimlibs. Port of the IMAP resource to batch
> notifications is in akonadi-batch-notifications branch in kdepim-runtime. I
> will port additional agents and resources (where it makes sense), but I
> want to make sure every one is happy with the API changes.
> 
> The code is backwards compatible, so you can run Akonadi server and
> kdepimlibs with batch-notifications, but kdepim-runtime from master without
> any problems.
> 
> I've been running the batch-notification branches on my production setup for
> over a week now without any troubles, so I think it's time to get it
> reviewed
> :-)
> 
> 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).

NotificationMessageV2::List should probably be a QVector, we couldn't change 
that in V1 anymore.

NotificationMessageV2::Item should probably be called "Entity" if it also can 
represent Collections.


> Akonadi::Monitor
> The set of signals that Monitor can emit has been expanded by itemsMoved,
> itemsRemoved, itemsLinked, itemsUnlinked and itemsFlagsChanged.  Arguably,
> batch operations for collections could be added, but I don't see any gain
> here because such use case is extremely rare, if any.
> Before emitting the signal, Monitor checks whether there's anyone listening
> to the old single-item signals (itemMoved, etc) and in such case, it splits
> the notification into many per-item notifications and emits them the old
> way. If there someone listening to the batch signals as well, the original
> batch notification is emitted too.
> 
> 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?

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?

> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20130518/ba71efe1/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/