[Kde-pim] Review Request 111885: Avoid starting a database transaction in CollectionSync when no updates are needed

[Dan Vrátil]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111885/#review37264
-----------------------------------------------------------


Thanks. 

There are some more conding style nitpicks (sorry for missing them in the first round) and, more importantly, I'm curious whether it's really necessary to restart the job when you detect that a change is needed and create a transaction (see the inline comments)


akonadi/collectionsync.cpp
<http://git.reviewboard.kde.org/r/111885/#comment27567>

    else on the same line as }



akonadi/collectionsync.cpp
<http://git.reviewboard.kde.org/r/111885/#comment27568>

    & should be at the variable name



akonadi/collectionsync.cpp
<http://git.reviewboard.kde.org/r/111885/#comment27569>

    space around ( and )



akonadi/collectionsync.cpp
<http://git.reviewboard.kde.org/r/111885/#comment27570>

    Space around ( ) and around "=" operator



akonadi/collectionsync.cpp
<http://git.reviewboard.kde.org/r/111885/#comment27571>

    & at variable name, spaces around "=" operator



akonadi/collectionsync.cpp
<http://git.reviewboard.kde.org/r/111885/#comment27572>

    spaces around ( and ) (twice)



akonadi/collectionsync.cpp
<http://git.reviewboard.kde.org/r/111885/#comment27573>

    Spaces around ( and )



akonadi/collectionsync.cpp
<http://git.reviewboard.kde.org/r/111885/#comment27574>

    Spaces around ( and )



akonadi/collectionsync.cpp
<http://git.reviewboard.kde.org/r/111885/#comment27575>

    Spaces around ( and )



akonadi/collectionsync.cpp
<http://git.reviewboard.kde.org/r/111885/#comment27576>

    Spaces around ( and )



akonadi/collectionsync.cpp
<http://git.reviewboard.kde.org/r/111885/#comment27581>

    Why do you need to re-fetch the collections? Fetch is read-only, so I don't think it has to happen again within the transaction.



akonadi/collectionsync.cpp
<http://git.reviewboard.kde.org/r/111885/#comment27577>

    Spaces around ( and )



akonadi/collectionsync.cpp
<http://git.reviewboard.kde.org/r/111885/#comment27582>

    This is related to my question about why you need to restart the job when you create a new transaction. Why can't the code execution simply continue here?
    
    If there's no update needed then good, the job has already called emitResult(), so you could just terminate here. But if there's an update needed, you have started the transaction already and you can now just start writing the changes to Akonadi without restarting the job.
    
    So the code could be somthing like
    
    // Check whether there's anything to update
    if ( !checkUpdateNecessity() ) {
      q->emitResult();
      return;
    }
    
    



akonadi/collectionsync.cpp
<http://git.reviewboard.kde.org/r/111885/#comment27578>

    Spaces around ( and )



akonadi/collectionsync.cpp
<http://git.reviewboard.kde.org/r/111885/#comment27579>

    Space around ( and ), wrap the statement in {}


- Dan Vrátil


On Aug. 5, 2013, 3:27 p.m., Alessandro Pignotti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111885/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2013, 3:27 p.m.)
> 
> 
> Review request for Akonadi.
> 
> 
> Description
> -------
> 
> Rationale: Unconditionally starting a transaction on every collection
> sync has an high overhead on disk activity since transaction bookkeping
> on the database requires writing to disk. Most often collection sync are
> unnecessary since collection are rarely modified, so we end up causing a
> write to disk on an operation which would be read-only.
> 
> The proposed solution is two run the collection sync in 2 phases:
> 
> 1) Without starting a transaction, the currently known local collections
> are loaded and are compared against the received remote ones. If no
> difference is found update is stopped right away
> 
> 2) If there are differences a transaction is started, local collections
> are loaded again to guarantee consistency and the update is applied
> inside the transaction
> 
> 
> This addresses bug 322083.
>     http://bugs.kde.org/show_bug.cgi?id=322083
> 
> 
> Diffs
> -----
> 
>   akonadi/collectionsync.cpp 0b85c2be168511486b484c7fcf8de3d157e876d4 
>   akonadi/collectionsync_p.h 2ea53c5e0f2d2ce8e1a684b8f685ab6683cd1493 
> 
> Diff: http://git.reviewboard.kde.org/r/111885/diff/
> 
> 
> Testing
> -------
> 
> Code builds and collection fetching seems to be working correctly
> 
> 
> Thanks,
> 
> Alessandro Pignotti
> 
>

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