On Thu, 2011-09-15 at 15:40 +0200, Lukas Zeller wrote:
> Hi Patrick,
>
> On Sep 12, 2011, at 20:33 , Patrick Ohly wrote:
>
> > The only remaining question is: should the merge take full advantage of
> > the age information available in this case ("newer wins") or merge data
> > on a per-property basis for those which support it (multi-line +
> > merge=lines)?
>
> What it exactly *should* do is probably depending on the reason why
> the plugin asks for merging, and would need to tweak the merge
> options / merge scripts such that it makes sense.
>
> What it does so far is only a merge, with fixed roles: the incoming
> item is the "winning" side, the DB item is the "loosing" side, without
> checking age information. Without a merge script, what happens is what
> TMultiFieldItem::standardMergeWith() does, which is
> based on the "merge" option set in the field list.
I think that makes sense, except for the "fixed roles" - more on that
below.
> > What I see is right now is that for SUMMARY, the most recent value is
> > used (good!).
>
> It's just the value coming from the remote, most recent or not.
Then it happened to work as expected accidentally, or I wasn't looking
closely enough.
> Because "SUMMARY" has no field-level merge option, which means that
> the winning side overwrites the loosing side.
> Of course, in the "most recently made known to the recipient", the
> incoming item is the most recent one by definition, but not
> necessarily in terms of when the user last edited it.
I think in this particular case, iCalendar 2.0 with UID, doing an age
comparison by default would be the right choice. I have implemented
that, see attached patch. Does that look right?
Note that it also avoids local DB updates which were found to be
unnecessary because the local side wasn't changed.
This does not yet address the situation where the remote side also
wasn't changed. A redundant Replace command will be sent in that case.
Do you have a hint for me how this can be avoided? In the server case,
is it as simple as avoiding the SendItemAsServer(augmentedItemP) call?
> Would it make sense to add an option to configure 509-merge run in one
> of "DB wins", "incomin wins", "newer wins" modes?
SyncEvolution doesn't need it at the moment, if you agree that the age
comparison should be used by default.
> > In the case of iCalendar 2.0 I wonder whether merge=lines is needed.
> > For modify/modify conflicts perhaps, but not so much for add/add
> > conflicts. Thoughts?
>
> If you want to differentiate these cases, I'd say we'll need a
> MERGEREASON() script function so the script can act differently
> depending on why the merge is initiated. So far I see the following
> different reasons: slowsync non-perfect match, syncml update conflict,
> DB update conflict, DB add conflict.
I'm inclined to rather remove the merge=lines and thus keep properties
from the winning side in all cases.
Except... what if a dumb phone truncates a property and then a slow sync
is done? Will the default merge mode preserve the longer property or
truncate if the version on the phone happens to be considered "winning"?
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
>From 2fa26d7027686990bf89ace5fa0699e0dca8377f Mon Sep 17 00:00:00 2001
From: Patrick Ohly <[email protected]>
Date: Fri, 30 Sep 2011 11:13:56 +0200
Subject: [PATCH 1/3] DB_Conflict (409): do age comparison before merging, avoid unneeded changes
Instead of always assuming that the incoming item is meant to win,
look at the age of the two items first.
Also tell the code which updates the DB item whether it was changed at
all and skip the DB write if it is not needed. Note that statistics
are still wrong: a server add is counted as "item added" in all cases,
even if it is turned into an update or becomes a nop.
---
src/sysync/customimplds.cpp | 65 ++++++++++++++++++++++++++++++++++--------
src/sysync/customimplds.h | 2 +-
2 files changed, 53 insertions(+), 14 deletions(-)
diff --git a/src/sysync/customimplds.cpp b/src/sysync/customimplds.cpp
index f6ed50a..9445391 100755
--- a/src/sysync/customimplds.cpp
+++ b/src/sysync/customimplds.cpp
@@ -2718,9 +2718,12 @@ localstatus TCustomImplDS::implProcessMap(cAppCharP aRemoteID, cAppCharP aLocalI
-/// helper to merge database version of an item with the passed version of the same item
-TMultiFieldItem *TCustomImplDS::mergeWithDatabaseVersion(TSyncItem *aSyncItemP)
+/// helper to merge database version of an item with the passed version of the same item;
+/// does age comparison by default, with "local side wins" as fallback
+TMultiFieldItem *TCustomImplDS::mergeWithDatabaseVersion(TSyncItem *aSyncItemP, bool &aChangedDBVersion)
{
+ aChangedDBVersion = false;
+
TStatusCommand dummy(fSessionP);
TMultiFieldItem *dbVersionItemP = (TMultiFieldItem *)newItemForRemote(aSyncItemP->getTypeID());
if (!dbVersionItemP) return NULL;
@@ -2733,13 +2736,39 @@ TMultiFieldItem *TCustomImplDS::mergeWithDatabaseVersion(TSyncItem *aSyncItemP)
bool ok=logicRetrieveItemByID(*dbVersionItemP,dummy);
if (ok && dummy.getStatusCode()!=404) {
// item found in DB, merge with original item
- bool changedNewVersion, changedDBVersion;
- aSyncItemP->mergeWith(*dbVersionItemP, changedNewVersion, changedDBVersion, this);
- PDEBUGPRINTFX(DBG_DATA,(
- "Merged incoming item (winning,relevant,%smodified) with version from database (loosing,to-be-replaced,%smodified)",
- changedNewVersion ? "" : "NOT ",
- changedDBVersion ? "" : "NOT "
- ));
+ // TODO (?): make this configurable
+ TConflictResolution crstrategy = cr_newer_wins;
+
+ if (crstrategy==cr_newer_wins) {
+ sInt16 cmpRes = aSyncItemP->compareWith(*dbVersionItemP,
+ eqm_nocompare,this
+#ifdef SYDEBUG
+ ,PDEBUGTEST(DBG_CONFLICT+DBG_DETAILS) // show age comparisons only if we want to see details
+#endif
+ );
+ if (cmpRes==1) crstrategy=cr_server_wins;
+ else crstrategy=cr_client_wins;
+ PDEBUGPRINTFX(DBG_DATA,(
+ "Newer item determined: %s",
+ crstrategy==cr_client_wins ?
+ "Incoming item is newer and wins" :
+ "DB item is newer and wins"));
+
+ bool changedNewVersion;
+ if (crstrategy==cr_client_wins) {
+ aSyncItemP->mergeWith(*dbVersionItemP, changedNewVersion, aChangedDBVersion, this);
+ } else {
+ dbVersionItemP->mergeWith(*aSyncItemP, aChangedDBVersion, changedNewVersion, this);
+ }
+ PDEBUGPRINTFX(DBG_DATA,(
+ "Merged incoming item (%s,relevant,%smodified) with version from database (%s,%s,%smodified)",
+ crstrategy==cr_client_wins ? "winning" : "loosing",
+ changedNewVersion ? "" : "NOT ",
+ crstrategy==cr_server_wins ? "winning" : "loosing",
+ aChangedDBVersion ? "to-be-replaced" : "to-be-left-unchanged",
+ aChangedDBVersion ? "" : "NOT "
+ ));
+ }
}
else {
// no item found, we cannot force a conflict
@@ -2834,11 +2863,16 @@ bool TCustomImplDS::implProcessItem(
if (sta==DB_Conflict) {
// DB has detected item conflicts with data already stored in the database and
// request merging current data from the backend with new data before storing.
- augmentedItemP = mergeWithDatabaseVersion(myitemP);
+ bool changedDBVersion;
+ augmentedItemP = mergeWithDatabaseVersion(myitemP, changedDBVersion);
if (augmentedItemP==NULL)
sta = DB_Error; // no item found, DB error
else {
- sta = apiUpdateItem(*augmentedItemP); // store augmented version back to DB
+ // store augmented version back to DB only if modified
+ if (changedDBVersion)
+ sta = apiUpdateItem(*augmentedItemP);
+ else
+ sta = LOCERR_OK;
// in server case, further process like backend merge (but no need to fetch again, we just keep augmentedItemP)
if (IS_SERVER && sta==LOCERR_OK) sta = DB_DataMerged;
}
@@ -2944,11 +2978,16 @@ bool TCustomImplDS::implProcessItem(
if (sta==DB_Conflict) {
// DB has detected item conflicts with data already stored in the database and
// request merging current data from the backend with new data before storing.
- augmentedItemP = mergeWithDatabaseVersion(myitemP);
+ bool changedDBVersion;
+ augmentedItemP = mergeWithDatabaseVersion(myitemP, changedDBVersion);
if (augmentedItemP==NULL)
sta = DB_Error; // no item found, DB error
else {
- sta = apiUpdateItem(*augmentedItemP); // store augmented version back to DB
+ // store augmented version back to DB only if modified
+ if (changedDBVersion)
+ sta = apiUpdateItem(*augmentedItemP);
+ else
+ sta = LOCERR_OK;
delete augmentedItemP; // forget now
}
}
diff --git a/src/sysync/customimplds.h b/src/sysync/customimplds.h
index 0437866..939f1ad 100755
--- a/src/sysync/customimplds.h
+++ b/src/sysync/customimplds.h
@@ -773,7 +773,7 @@ protected:
// as a item copy with only finalisation-required fields
void queueForFinalisation(TMultiFieldItem *aItemP);
/// helper to merge database version of an item with the passed version of the same item
- TMultiFieldItem *mergeWithDatabaseVersion(TSyncItem *aSyncItemP);
+ TMultiFieldItem *mergeWithDatabaseVersion(TSyncItem *aSyncItemP, bool &aChangedDBVersion);
public:
// - get last to-remote sync time
lineartime_t getPreviousToRemoteSyncCmpRef(void) { return fPreviousToRemoteSyncCmpRef; };
--
1.7.2.5
_______________________________________________
os-libsynthesis mailing list
[email protected]
http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis
