[Kde-pim] Review Request 117471: ItemSync: Use batch processing and allow throttling

[Christian Mollekopf]

> On April 15, 2014, 12:10 p.m., Dan Vrátil wrote:
> > Damn good job! And sorry for taking so long time with the review.

Thanks =) And thanks for the review. I'll address your other points.


> On April 15, 2014, 12:10 p.m., Dan Vrátil wrote:
> > akonadi/itemsync.h, line 173
> > <https://git.reviewboard.kde.org/r/117471/diff/1/?file=263949#file263949line173>
> >
> >     Should the batch size be configurable? Maybe some resources can handle larger batches thanks to cheaper payload parsing, or because they use local storage and can access it faster than remote resources.
> >     
> >     Does this have to be static? I'd prefer to have regular const member method rather than static. I assume it's static so that ResourceBase::batchSize() can be called even when there's no ItemSync, but it can be documented that ResourceBase::batchSize() returns valid batch size only during item synchronization. Or maybe ResourceBase does not need to expose that information at all...?

The problem is that the ItemSync instance is not existing yet when the sync is started, but only once the resources starts to deliver the items, and I was being lazy to search for the proper solution ;-)

It needs to be exposed because the resource needs to deliver batchSize() items and then wait for the signal that it can deliver more items.

So what we could do is either store the batchSize in resource base (which I don't like but would work), or create the ItemSerializer earlier so it actually is available from the start when retrieveItems is called.


> On April 15, 2014, 12:10 p.m., Dan Vrátil wrote:
> > akonadi/itemsync.cpp, line 471
> > <https://git.reviewboard.kde.org/r/117471/diff/1/?file=263950#file263950line471>
> >
> >     What if the last batch is smaller than batchSize? Won't this cause the syncer to wait indefinitely?

Yes, which I think is not avoidable, except with some arcane timeout, which I'd rather avoid.


- Christian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117471/#review55795
-----------------------------------------------------------


On April 10, 2014, 9:20 a.m., Christian Mollekopf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117471/
> -----------------------------------------------------------
> 
> (Updated April 10, 2014, 9:20 a.m.)
> 
> 
> Review request for KDEPIM-Libraries.
> 
> 
> Repository: kdepimlibs
> 
> 
> Description
> -------
> 
> ItemSync: Use batch processing and allow throttling
> 
> With this patch we process the items in batches and gives resources the
> possiblity to throttle the retrieval of messages using the
> readyForNextBatch signal. This way we avoid using huge queries and
> loading all items into memory, making the whole process a lot less
> resource intensive, and make it work for large folder.
> 
> 
> Diffs
> -----
> 
>   akonadi/itemsync.h 32166d7154b0a9a5131b0dd215c0a5a75b2a67dd 
>   akonadi/itemsync.cpp 421c14a027836800bab1e62ca466d68475c71e1f 
>   akonadi/resourcebase.h 01f86a84c5fd78b8dfcabf638a881759b159f04c 
>   akonadi/resourcebase.cpp 4e5b1e9289d1cbfba42490961dab78ebaad8a586 
>   akonadi/tests/itemsynctest.cpp 4999c03abb25f3e1cf767b287dcf7698693745e9 
> 
> Diff: https://git.reviewboard.kde.org/r/117471/diff/
> 
> 
> Testing
> -------
> 
> I adapted the imap resource accordingly and I can now sync 200k mail folder with the bottleneck being mysql (as it should) and no significant cpu or memory usage by akonadi-server or the imap resource. Also, unittests. 
> 
> 
> Thanks,
> 
> Christian Mollekopf
> 
>

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