[RFC] drop plugin batch_commit for 0.40?

[Michael Bell]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

first this issue is tracked by ticket #1078.

http://opensync.org/ticket/1078

OpenSync supports two ways of writing changes to a plugin -
commit_change and batch_commit.

The first variant calls a callback for every change and optionally if
requested by the plugin then committed_all callback is called. This
means one callback per change and an optional all changes done callback.

The second variant calls only one callback and submits all changes at
once. All changes still have an own context but all changes are
collected first.

The second variant cause a problem with our IPC message queues. IPC
queues should always be limited in some way. OpenSync uses such IPC
message queues to handle changes and the answers to these changes. Every
change is sent to a queue. If the queue handles the change then the
according handler is called and the pending reply to this message is put
into another queue. These pending replies are limited to avoid too many
waiting IPC messages. So back to two variants. What happens?

The first variant immediately calls a callback for every change. The
change is handled and the context is signalled. If context is signalled
then the reply is present and no longer pending. The IPC queue can
handle the next message (which is a change or the committed_all
message). So the message queue does not block.

The second variant does not call a change callback per change message.
All changes are cached and when the committed_all message is handled
then all change messages are handled at once. If there are more then IPC
queue limit - 1 changes then the queue stops working until one of the
change messages is handled. No change message is handled until the
committed_all message is handled. So there is a deadlock.

Therefore I want to remove the batch commit function. If one change
fails and is already committed then committed_all or sync_done can be
used to signal an error which would cause a new slow-sync.

The SyncML mapping is an open issue.

SyncML/OMA DS clients can send a mapping after the changes from a server
were received. These mappings are first available after the changes are
committed. OpenSync is today not able to handle this after a change is
already committed. So we still need a solution for the mapping.

Best regards

Michael
- --
___________________________________________________________________

Michael Bell                        Humboldt-Universitaet zu Berlin

Tel.: [phone]           ZE Computer- und Medienservice
Fax:  [phone]           Unter den Linden 6
michael.bell@...       D-10099 Berlin
___________________________________________________________________

PGP Fingerprint: 09E4 3D29 4156 2774 0F2C  C643 D8BD 1918 2030 5AAB
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFJ3Ihi2L0ZGCAwWqsRAi00AJ978vdR/pj2vBvKnGnD/LDXnzvRBgCfU93n
h6deJufpf2hnWFpEW0SrGr4=
=8E8j
-----END PGP SIGNATURE-----

------------------------------------------------------------------------------
This SF.net email is sponsored by:
High Quality Requirements in a Collaborative Environment.
Download a free trial of Rational Requirements Composer Now!
http://p.sf.net/sfu/www-ibm-com
_______________________________________________
Opensync-devel mailing list
Opensync-devel@...
https://lists.sourceforge.net/lists/listinfo/opensync-devel

[Michael Bell]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Michael Bell wrote:
>
> The SyncML mapping is an open issue.
>
> SyncML/OMA DS clients can send a mapping after the changes from a server
> were received. These mappings are first available after the changes are
> committed. OpenSync is today not able to handle this after a change is
> already committed. So we still need a solution for the mapping.

I could write some code for the SyncML plugin which writes the SyncML
mappings to the state_db of the sink because they are too late for the
changes. This gives us some time to think about a proper solution. In
the mean time we could already remove batch commit.

Best regards

Michael
- --
___________________________________________________________________

Michael Bell                        Humboldt-Universitaet zu Berlin

Tel.: [phone]           ZE Computer- und Medienservice
Fax:  [phone]           Unter den Linden 6
michael.bell@...       D-10099 Berlin
___________________________________________________________________

PGP Fingerprint: 09E4 3D29 4156 2774 0F2C  C643 D8BD 1918 2030 5AAB
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFJ3JrG2L0ZGCAwWqsRAgOqAKCMTD6O/l4ii2gZp29PCf+5MmVNGQCg1m++
8c6jWCKjOfmDrxfFnQoKQYo=
=/MdK
-----END PGP SIGNATURE-----

------------------------------------------------------------------------------
This SF.net email is sponsored by:
High Quality Requirements in a Collaborative Environment.
Download a free trial of Rational Requirements Composer Now!
http://p.sf.net/sfu/www-ibm-com
_______________________________________________
Opensync-devel mailing list
Opensync-devel@...
https://lists.sourceforge.net/lists/listinfo/opensync-devel

[Daniel Gollub-3]

On Wednesday 08 April 2009 02:38:30 pm Michael Bell wrote:
> Michael Bell wrote:
> > The SyncML mapping is an open issue.
> >
> > SyncML/OMA DS clients can send a mapping after the changes from a server
> > were received. These mappings are first available after the changes are
> > committed. OpenSync is today not able to handle this after a change is
> > already committed.

That's wrong. Plugins can change the Mapping ID for this particular peer after
it got commit to the peer.

Example:

- commit() call with change UID: 00001
- plugin calls protocl specific code to commit change to peer
- plugins retrieves the new ID of the change which got just committed. ID: 123
- plugin now needs to call osync_changet_set_uid(change, "123");
- then the commit function ends with osync_context_report_success()

With the context reply (osync_context_report_sucess()) an internal function
get called which reports the new UID to the internal mapping table.

> > So we still need a solution for the mapping.

I don't agree here - see above.

> I could write some code for the SyncML plugin which writes the SyncML
> mappings to the state_db of the sink because they are too late for the
> changes.

I really don't want to see that plugins reinvent the wheel and write their own
mapping table. And reverting such temp. solution might cost more time to
revert then actually find a final solution.

> This gives us some time to think about a proper solution. In
> the mean time we could already remove batch commit.

I still haven't understand the entire problem in detail. And have the feeling
that dropping batch_commit might be not the right thing todo before having a
solution which solves the entire issue.

Best Regards,
Daniel


--
Daniel Gollub                        Geschaeftsfuehrer: Ralph Dehner
FOSS Developer                       Unternehmenssitz:  Vohburg
B1 Systems GmbH                      Amtsgericht:       Ingolstadt
Mobil: [phone]         Handelsregister:   HRB 3537
EMail: gollub@...          http://www.b1-systems.de

Adresse: B1 Systems GmbH, Osterfeldstraße 7, 85088 Vohburg
http://pgpkeys.pca.dfn.de/pks/lookup?op=get&search=0xED14B95C2F8CA78D

------------------------------------------------------------------------------
This SF.net email is sponsored by:
High Quality Requirements in a Collaborative Environment.
Download a free trial of Rational Requirements Composer Now!
http://p.sf.net/sfu/www-ibm-com
_______________________________________________
Opensync-devel mailing list
Opensync-devel@...
https://lists.sourceforge.net/lists/listinfo/opensync-devel

[Daniel Gollub-3]

On Monday 13 April 2009 02:09:45 pm Daniel Gollub wrote:
> > This gives us some time to think about a proper solution. In
> > the mean time we could already remove batch commit.
>
> I still haven't understand the entire problem in detail. And have the
> feeling that dropping batch_commit might be not the right thing todo before
> having a solution which solves the entire issue.

Ok - i have to revoke the last line. Removing batch_commit for the api freeze
was right thing to do - until we have the queue limit issue on this kind of
plugin "batch" i/o resolved.

Best Regards,
Daniel

--
Daniel Gollub                        Geschaeftsfuehrer: Ralph Dehner
FOSS Developer                       Unternehmenssitz:  Vohburg
B1 Systems GmbH                      Amtsgericht:       Ingolstadt
Mobil: [phone]         Handelsregister:   HRB 3537
EMail: gollub@...          http://www.b1-systems.de

Adresse: B1 Systems GmbH, Osterfeldstraße 7, 85088 Vohburg
http://pgpkeys.pca.dfn.de/pks/lookup?op=get&search=0xED14B95C2F8CA78D

------------------------------------------------------------------------------
This SF.net email is sponsored by:
High Quality Requirements in a Collaborative Environment.
Download a free trial of Rational Requirements Composer Now!
http://p.sf.net/sfu/www-ibm-com
_______________________________________________
Opensync-devel mailing list
Opensync-devel@...
https://lists.sourceforge.net/lists/listinfo/opensync-devel

[Juergen Leising]

On Mon, Apr 13, 2009 at 02:09:45PM +0200, Daniel Gollub wrote:
 

> Example:
>
> - commit() call with change UID: 00001
> - plugin calls protocl specific code to commit change to peer
> - plugins retrieves the new ID of the change which got just committed. ID: 123
> - plugin now needs to call osync_changet_set_uid(change, "123");
> - then the commit function ends with osync_context_report_success()
>
> With the context reply (osync_context_report_sucess()) an internal function
> get called which reports the new UID to the internal mapping table.

Oh, that's interesting.  Did you know, that the file-sync plugin
changes the UID in file-sync/src/filename_scape.h   ?
Unfortunately, the filenames are the actual carrier for the UIDs.  

This causes some trouble in the LDAP plugin, which does not
expect such changes. I have tried a workaround in the LDAP plugin,
but I am not too happy with it.

So if I have understood you correctly, adding osync_changet_set_uid()
anywhere in the file-sync plugin is the solution for the problems
(which I do not fully overlook), isn't it?

Bye, bye,

Juergen



------------------------------------------------------------------------------
This SF.net email is sponsored by:
High Quality Requirements in a Collaborative Environment.
Download a free trial of Rational Requirements Composer Now!
http://p.sf.net/sfu/www-ibm-com
_______________________________________________
Opensync-devel mailing list
Opensync-devel@...
https://lists.sourceforge.net/lists/listinfo/opensync-devel

[Daniel Gollub-3]

On Monday 13 April 2009 05:18:44 pm Juergen Leising wrote:

> On Mon, Apr 13, 2009 at 02:09:45PM +0200, Daniel Gollub wrote:
> > Example:
> >
> > - commit() call with change UID: 00001
> > - plugin calls protocl specific code to commit change to peer
> > - plugins retrieves the new ID of the change which got just committed.
> > ID: 123 - plugin now needs to call osync_changet_set_uid(change, "123");
> > - then the commit function ends with osync_context_report_success()
> >
> > With the context reply (osync_context_report_sucess()) an internal
> > function get called which reports the new UID to the internal mapping
> > table.
>
> Oh, that's interesting.  Did you know, that the file-sync plugin
> changes the UID in file-sync/src/filename_scape.h   ?
> Unfortunately, the filenames are the actual carrier for the UIDs.

No - i missed that one. And you're right - thats causing a problem.
If filename_scape_characters() modifies the filename it's also required to
update the uid. I have prepared a patch - good you give it a try if this
solves the issue for your test secenario?

>
> This causes some trouble in the LDAP plugin, which does not
> expect such changes. I have tried a workaround in the LDAP plugin,
> but I am not too happy with it.
>
> So if I have understood you correctly, adding osync_changet_set_uid()
> anywhere in the file-sync plugin is the solution for the problems
> (which I do not fully overlook), isn't it?

Right. I hope my fix is doing this. Please let me know if this is working..

(I refactored the file-scape function call - so the diff got a big more
complex. There is no need to scape twice. Since there is not sink "write"
function anylonger. So currently it's guranteed that only the filesync_commit
function calls the _write() function).

Thanks for the hint!

Best Regards,
Daniel

--
Daniel Gollub                        Geschaeftsfuehrer: Ralph Dehner
FOSS Developer                       Unternehmenssitz:  Vohburg
B1 Systems GmbH                      Amtsgericht:       Ingolstadt
Mobil: [phone]         Handelsregister:   HRB 3537
EMail: gollub@...          http://www.b1-systems.de

Adresse: B1 Systems GmbH, Osterfeldstraße 7, 85088 Vohburg
http://pgpkeys.pca.dfn.de/pks/lookup?op=get&search=0xED14B95C2F8CA78D

[file-sync_uid_fix.diff]

---

Index: src/filename_scape.h
===================================================================
--- src/filename_scape.h (revision 5596)
+++ src/filename_scape.h (working copy)
@@ -28,15 +28,17 @@
 static const int reserved_count = 9;
 static const char scaper = '_';
 
-static void filename_scape_characters(char *input)
+static osync_bool filename_scape_characters(char *input)
 {
 
  int i;
+ osync_bool modified = FALSE;
 
  while (*input) {
  for (i = 0; i < reserved_count; ++i)
  if (*input ==  reserved_chars[i]) {
  *input++ = scaper;
+ modified = TRUE;
  goto done;
  }
  ++input;
@@ -44,6 +46,7 @@
  ;
  }
 
+ return modified;
 }
 
 #endif
Index: src/file_sync.c
===================================================================
--- src/file_sync.c (revision 5596)
+++ src/file_sync.c (working copy)
@@ -173,15 +173,9 @@
  char *buffer = NULL;
  unsigned int size = 0;
 
- char *filename = NULL, *tmp = NULL;
- if (!(tmp = strdup(osync_change_get_uid(change))))
- goto error;
+ char *filename = NULL;
+ filename = g_strdup_printf ("%s%c%s", dir->path, G_DIR_SEPARATOR, osync_change_get_uid(change));
 
- filename_scape_characters(tmp);
-
- filename = g_strdup_printf ("%s%c%s", dir->path, G_DIR_SEPARATOR, tmp);
- free(tmp);
-
  switch (osync_change_get_changetype(change)) {
  case OSYNC_CHANGE_TYPE_DELETED:
  if (!remove(filename) == 0) {
@@ -453,22 +447,25 @@
  OSyncFileDir *dir = userdata;
  OSyncHashTable *hashtable = osync_objtype_sink_get_hashtable(sink);
 
+ char *hash = NULL;
  char *filename = NULL, *tmp;
-
- if (!osync_filesync_write(sink, info, ctx, change, userdata)) {
- osync_trace(TRACE_EXIT_ERROR, "%s", __func__);
- return;
- }
+
  if (!(tmp = strdup(osync_change_get_uid(change)))) {
  osync_trace(TRACE_EXIT_ERROR, "%s", __func__);
  return;
  }
- filename_scape_characters(tmp);
 
- filename = g_strdup_printf ("%s/%s", dir->path, tmp);
+ if (filename_scape_characters(tmp))
+ osync_change_set_uid(change, tmp);
+
+ filename = g_strdup_printf ("%s%c%s", dir->path, G_DIR_SEPARATOR, tmp);
  free(tmp);
- char *hash = NULL;
 
+ if (!osync_filesync_write(sink, info, ctx, change, userdata)) {
+ osync_trace(TRACE_EXIT_ERROR, "%s", __func__);
+ return;
+ }
+
  if (osync_change_get_changetype(change) != OSYNC_CHANGE_TYPE_DELETED) {
  struct stat buf;
  stat(filename, &buf);

---

------------------------------------------------------------------------------
This SF.net email is sponsored by:
High Quality Requirements in a Collaborative Environment.
Download a free trial of Rational Requirements Composer Now!
http://p.sf.net/sfu/www-ibm-com
_______________________________________________
Opensync-devel mailing list
Opensync-devel@...
https://lists.sourceforge.net/lists/listinfo/opensync-devel

[Juergen Leising]

On Mon, Apr 13, 2009 at 05:44:22PM +0200, Daniel Gollub wrote:
(...)

> No - i missed that one. And you're right - thats causing a problem.
> If filename_scape_characters() modifies the filename it's also required to
> update the uid. I have prepared a patch - good you give it a try if this
> solves the issue for your test secenario?
>
> >
> > This causes some trouble in the LDAP plugin, which does not
> > expect such changes. I have tried a workaround in the LDAP plugin,
> > but I am not too happy with it.
> >
> > So if I have understood you correctly, adding osync_changet_set_uid()
> > anywhere in the file-sync plugin is the solution for the problems
> > (which I do not fully overlook), isn't it?
>
> Right. I hope my fix is doing this. Please let me know if this is working..

(...)

Yes, it does work for me.  The tests were successful.  Many
thanks.



------------------------------------------------------------------------------
This SF.net email is sponsored by:
High Quality Requirements in a Collaborative Environment.
Download a free trial of Rational Requirements Composer Now!
http://p.sf.net/sfu/www-ibm-com
_______________________________________________
Opensync-devel mailing list
Opensync-devel@...
https://lists.sourceforge.net/lists/listinfo/opensync-devel

[Juha Tuomala-2]

Some parts of this message have been removed. Learn more about Nabble's security policy.

On Monday 13 April 2009 18:18:44 Juergen Leising wrote:
> On Mon, Apr 13, 2009 at 02:09:45PM +0200, Daniel Gollub wrote:
>
> > Example:
> >
> > - commit() call with change UID: 00001
> > - plugin calls protocl specific code to commit change to peer
> > - plugins retrieves the new ID of the change which got just committed. ID: 123
> > - plugin now needs to call osync_changet_set_uid(change, "123");
> > - then the commit function ends with osync_context_report_success()
> >
> > With the context reply (osync_context_report_sucess()) an internal function
> > get called which reports the new UID to the internal mapping table.
>
> Oh, that's interesting. Did you know, that the file-sync plugin
> changes the UID in file-sync/src/filename_scape.h ?
> Unfortunately, the filenames are the actual carrier for the UIDs.

I tried to run file-plugin against kdepim4 Directory resource but it
failed due my misunderstanding of vcard versions, I used 2.1 in file-sync
and KDE uses 3.0 by default. After learning that opensync also supports
3.0, I need to try again it some day.

In KDE too, the UIDs are stored inside the file and into file name
and they do have to match, otherwise it will cause problems. I noticed
that the changes to the addressbook wont get written into disk if
they don't match. So those cards that came from phone, were unusable
in KDE side.

I think such conflict should be avoided regardless of the intended
use cases and other plugins.

Tuju

--
Varo hattupäisiä autoilijoita.

------------------------------------------------------------------------------
This SF.net email is sponsored by:
High Quality Requirements in a Collaborative Environment.
Download a free trial of Rational Requirements Composer Now!
http://p.sf.net/sfu/www-ibm-com
_______________________________________________
Opensync-devel mailing list
Opensync-devel@...
https://lists.sourceforge.net/lists/listinfo/opensync-devel

[Michael Bell]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Daniel Gollub wrote:

> On Wednesday 08 April 2009 02:38:30 pm Michael Bell wrote:
>> Michael Bell wrote:
>>> The SyncML mapping is an open issue.
>>>
>>> SyncML/OMA DS clients can send a mapping after the changes from a server
>>> were received. These mappings are first available after the changes are
>>> committed. OpenSync is today not able to handle this after a change is
>>> already committed.
>
> That's wrong. Plugins can change the Mapping ID for this particular peer after
> it got commit to the peer.
>
> Example:
>
> - commit() call with change UID: 00001
> - plugin calls protocl specific code to commit change to peer
> - plugins retrieves the new ID of the change which got just committed. ID: 123
> - plugin now needs to call osync_changet_set_uid(change, "123");
> - then the commit function ends with osync_context_report_success()
>
> With the context reply (osync_context_report_sucess()) an internal function
> get called which reports the new UID to the internal mapping table.

The problem is that if you do SyncML this way then you run into a dead
lock after five commit() calls. "Committed changes" in my first mail
means that osync_context_report_success was called on the context of the
commit callback (of the plugin API). The pendingLimit stops the IPC
queue after five unreplied changes.

commit() is called for the UID 00001 to 00005 but no SyncML message is
perhaps send and so no map can be received but the IPC queue blocks in
this situation because five messages wait for a reply.

>> I could write some code for the SyncML plugin which writes the SyncML
>> mappings to the state_db of the sink because they are too late for the
>> changes.
>
> I really don't want to see that plugins reinvent the wheel and write their own
> mapping table. And reverting such temp. solution might cost more time to
> revert then actually find a final solution.

I don't want to do this. Please see my other RFC. The dead lock issue is
a fact.

Best regards

Michael
- --
___________________________________________________________________

Michael Bell                        Humboldt-Universitaet zu Berlin

Tel.: [phone]           ZE Computer- und Medienservice
Fax:  [phone]           Unter den Linden 6
michael.bell@...       D-10099 Berlin
___________________________________________________________________

PGP Fingerprint: 09E4 3D29 4156 2774 0F2C  C643 D8BD 1918 2030 5AAB
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFJ5HDD2L0ZGCAwWqsRAi1tAJ4mlmdRCmlMscrZEnnXxzcQJNCo2gCg6Jyi
2Qgg+ueQ0XB04142ukH/GNM=
=Ztd9
-----END PGP SIGNATURE-----

------------------------------------------------------------------------------
This SF.net email is sponsored by:
High Quality Requirements in a Collaborative Environment.
Download a free trial of Rational Requirements Composer Now!
http://p.sf.net/sfu/www-ibm-com
_______________________________________________
Opensync-devel mailing list
Opensync-devel@...
https://lists.sourceforge.net/lists/listinfo/opensync-devel