[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/#review37115
-----------------------------------------------------------


Thanks for the patch. This is a first round of review, I haven't checked the actual logic of the code, it's mostly just about coding style and general code issues.

I'm most concerned about the "TODO" for attribute change checking. The TODO is kinda problem, because without actual implementation, you would be ignoring changes in attribute content. In theory you could just call Attribute::serialized() on both attributes and compare the resulting QByteArrays, not sure how much that would impact performance however.






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

    Put { on the same line as if() and add spaces between the parentheses



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

    Add spaces between parentheses and variable



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

    Brackets on the same line as foreach



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

    Use kDebug(), fprintf is evil (and can't be silenced) :)



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

    Use { } even for one-line expressions and put spaces around the outer parentheses



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

    {}



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

    else on the same line as "}", and use { }



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

    s/is/if/



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

    You can immediately "return true" here, there's no need for further checks: no matter what their result will be, you will return "true" anyway at the end



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

    Add {}, add spaces after ";" and around the outer parentheses



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

    Same here, just "return true"



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

    I would change this (and the following few statements) into
    
    if ( localCollection.property() != removeCollection.property() ) {
      return true;
    }
    



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

    



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

    } else {
      return true;
    }



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

    This can all be replaces just by "return false". If the code execution gets this far, it means that the collections are identical



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

    Use bool-like "!currentTransaction" instead of NULL



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

    ( false )



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

    Same here, "!currentTransaction"



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

    Too many tabs?



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

    Use kDebug()



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

    CollectionFetchJob takes QObject* as the last argument, so no need to cast it to Job*



akonadi/collectionsync_p.h
<http://git.reviewboard.kde.org/r/111885/#comment27466>

    s/nop/noop/


- Dan Vrátil


On Aug. 5, 2013, 1:48 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, 1:48 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/