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

[Dan Vrátil]

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


Damn good job! And sorry for taking so long time with the review. 


akonadi/itemsync.h
<https://git.reviewboard.kde.org/r/117471/#comment38825>

    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...?



akonadi/itemsync.h
<https://git.reviewboard.kde.org/r/117471/#comment38826>

    Add @since and document properly the int argument.



akonadi/itemsync.h
<https://git.reviewboard.kde.org/r/117471/#comment38832>

    const &



akonadi/itemsync.cpp
<https://git.reviewboard.kde.org/r/117471/#comment38833>

    No more spaces inside parenthesis :-)



akonadi/itemsync.cpp
<https://git.reviewboard.kde.org/r/117471/#comment38834>

    Constify the method.



akonadi/itemsync.cpp
<https://git.reviewboard.kde.org/r/117471/#comment38819>

    Be more descriptive please, like "ItemSync is in error state, not modifying items"



akonadi/itemsync.cpp
<https://git.reviewboard.kde.org/r/117471/#comment38823>

    Check for fetchJob.items()->size() > 0 to prevent crash in case something goes wrong.



akonadi/itemsync.cpp
<https://git.reviewboard.kde.org/r/117471/#comment38835>

    No spaces inside ()



akonadi/itemsync.cpp
<https://git.reviewboard.kde.org/r/117471/#comment38853>

    The ItemsFetchJobs have EmitItemsIndividually set, so there's no need for foreach, just Q_ASSERT(items.count() == 1);



akonadi/itemsync.cpp
<https://git.reviewboard.kde.org/r/117471/#comment38854>

    What if the last batch is smaller than batchSize? Won't this cause the syncer to wait indefinitely?



akonadi/itemsync.cpp
<https://git.reviewboard.kde.org/r/117471/#comment38836>

    Both modifyLocalItem() and createLocalItem() take const reference to remoteItem, so you can switch to using const& here (and remove the comment).



akonadi/itemsync.cpp
<https://git.reviewboard.kde.org/r/117471/#comment38855>

    Theoretically, you might want to handle situation when there are two items with the same RID in one collection? (just a thought)



akonadi/itemsync.cpp
<https://git.reviewboard.kde.org/r/117471/#comment38857>

    Shouldn't this call checkDone() before returning?



akonadi/resourcebase.h
<https://git.reviewboard.kde.org/r/117471/#comment38839>

    Could you make the documentation more descriptive about how this can be used to throttle the delivery?
    
    s/akonadi/Akonadi/ ;-)



akonadi/resourcebase.h
<https://git.reviewboard.kde.org/r/117471/#comment38838>

    Document and name the int argument.
    
    Since this is in ResourceBase, it might not be clear what batch this refers to (could be notification batch), so I'd rename it to retrievenNextItemSyncBatch (or something in that sense)



akonadi/resourcebase.h
<https://git.reviewboard.kde.org/r/117471/#comment38840>

    Add documentation and @since please.
    
    See above for the name - maybe itemSyncBatchSize?
    
    And as mentioned in the ItemSync part, should the batch size be configurable?



akonadi/tests/itemsynctest.cpp
<https://git.reviewboard.kde.org/r/117471/#comment38841>

    If this assert is not valid anymore, then just remove it.



akonadi/tests/itemsynctest.cpp
<https://git.reviewboard.kde.org/r/117471/#comment38847>

    Add {} please


- Dan Vrátil


On April 10, 2014, 11: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, 11: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/