Community
Participate
Working Groups
Summary says it all. I also believe Scott was interested in abstracting out some kind of API around sending changes (or something like that). This would be a step forward in that direction in my opinion.
(In reply to comment #0) > Summary says it all. > > I also believe Scott was interested in abstracting out some kind of API around > sending changes (or something like that). This would be a step forward in that > direction in my opinion. > Setting target milestone to 2.1. I would like to ultimately have an API for synchronizing changes to replicated models...so that people can/could plugin synchronization strategies (e.g. cola for text document changes). It would then be convenient to use the framework and extensions for synchronizing other sorts of models, using strategies appropriate for the model type.
Quite some time ago, I spent some time trying to define an API like this, not sure if it is helpful: have a look at org.eclipse.ecf.datashare2 in /cvsroot/technology/org.eclipse.ecf/archive/plugins
(In reply to comment #2) > Quite some time ago, I spent some time trying to define an API like this, not > sure if it is helpful: have a look at org.eclipse.ecf.datashare2 in > /cvsroot/technology/org.eclipse.ecf/archive/plugins > Hi Boris. Yes, I've been looking at datashare2. I think some of the main ideas will carry through...like IMarshallingStrategy and ISharedState...thanks.
Moving to cola component
Created attachment 113329 [details] Initial cut at new plugin to implement shared document synchronization
Added attachment #113329 [details] This is new plugin org.eclipse.ecf.sync to provide non-ui sync api and cola implementation for shared document synchronization (for use in docshare and/or others) What this plugin does is: 1) Creates some new API...i.e. IDocumentSynchronizationStrategy, IDocumentChange, IDocumentChangeMessage, (new) IDocumentSynchronizationStrategyFactory 2) Reorganizes cola classes in internal packages, and implements the API from 1 3) Sets up a IDocumentSynchronizationStrategyFactory instance as OSGi service (in Activator) so that other plugins can get the strategy factory and create instances of IDocumentSynchronizationStrategy for themselves as needed (via uniqueID and boolean isInitiator flag indicating whether the creator is the initiator).
Created attachment 113383 [details] Replacement plugin project. Added SerializationException New version of plugin project. Added SerializationException to API so that API methods do not expose Exception.
Created attachment 113394 [details] Even better version. Improved consistency of package naming, .options file, etc. Changes to the class/package naming for consistency. Now have document sync strategy API code in org.eclipse.ecf.sync.doc.* package(s).
(In reply to comment #8) > Created an attachment (id=113394) [details] > Even better version. Improved consistency of package naming, .options file, > etc. > > Changes to the class/package naming for consistency. Now have document sync > strategy API code in org.eclipse.ecf.sync.doc.* package(s). > This code has been checked into the new ECF repository (in runtime). See (CVS) /cvsroot/rt/org.eclipse.ecf/framework/bundles/org.eclipse.ecf.sync and /cvsroot/rt/org.eclipse.ecf/tests/bundles/org.eclipse.ecf.tests.sync
Missing license headers: org.eclipse.ecf.internal.sync.Activator org.eclipse.ecf.sync.IModelChange org.eclipse.ecf.sync.IModelSynchronizationStrategy API are not spec'ed with @since 2.1. org.eclipse.ecf.sync.IModelChangeMessage: -serialize() method has no javadoc org.eclipse.ecf.sync.IModelSynchronizationStrategy: -transformRemoteChange(IModelChange) @return is incorrectly spec'ed to return IDocumentChange[] org.eclipse.ecf.sync.doc.IDocumentSynchronizationStrategyFactory: -dispose() method has no javadoc org.eclipse.ecf.sync.doc.DocumentChangeMessage: -missing class-level and constructor javadoc (and such), not all that important though I suppose So if I'm correct I'm supposed to deserializeRemoteChange then from that transformRemoteChange?
Hi Remy, Thanks for the proofing. (In reply to comment #10) > Missing license headers: > org.eclipse.ecf.internal.sync.Activator > org.eclipse.ecf.sync.IModelChange > org.eclipse.ecf.sync.IModelSynchronizationStrategy > > API are not spec'ed with @since 2.1. > > org.eclipse.ecf.sync.IModelChangeMessage: > -serialize() method has no javadoc > > org.eclipse.ecf.sync.IModelSynchronizationStrategy: > -transformRemoteChange(IModelChange) @return is incorrectly spec'ed to return > IDocumentChange[] All of above fixed. > > org.eclipse.ecf.sync.doc.IDocumentSynchronizationStrategyFactory: > -dispose() method has no javadoc Removed dispose method in API. Not needed (responsibility for factory dispose is in the implementer/provider, not the clients). > > org.eclipse.ecf.sync.doc.DocumentChangeMessage: > -missing class-level and constructor javadoc (and such), not all that important > though I suppose Javadocs added. > > So if I'm correct I'm supposed to deserializeRemoteChange then from that > transformRemoteChange? > Yes, that's right. The intention here is to separate the serialization/deserialization from the operational transformation rather than bind them together (e.g. via one transform method...e.g. IModelChange transform(byte []). Cost is obviously extra method in API, but it seems worth it.
(In reply to comment #11) > > So if I'm correct I'm supposed to deserializeRemoteChange then from that > > transformRemoteChange? > > > > Yes, that's right. The intention here is to separate the > serialization/deserialization from the operational transformation rather than > bind them together (e.g. via one transform method...e.g. IModelChange > transform(byte []). Cost is obviously extra method in API, but it seems worth > it. I too feel that this makes the "actions that are being performed" more explicit to the API users (they know what's going on) and outweighs the extra method cost.
Keeping in mind the idea of don't affect the org.eclipse.ecf.docshare code on CVS/HEAD, I created a branch for this called sync_api_refactor. Now I'm working to remove the Docshare copy of the cola code, and replace it with use of the sync API (using cola behind the scenes). Following the suggestion made by Scott, I'm starting on: 1) remove the packages/classes that implement the current docshare synchronization strategy 2) Use/access the IDocumentSynchronizationStrategyFactory service (that is registered by the org.eclipse.ecf.sync plugin Activator) to create an ISynchronizationStrategy instance from within the DocShare class. ' 3) Replace the existing synchronization strategy calls with calls to the ISynchronizationStrategy instance created via IDocumentSynchronizationStrategyFactory.createSynchronizationStrategy 4) Test the new DocShare/ISynchronizationStrategy 5) Note that the new DocShare will *not* interoperate with the old/existing docshare unless we explicitly attempt to figure out allowing them to interoperate...
The branch sync_api_refactor is keeping the first "version" of DocShare without the Cola code. During this implementation I changed the sync API to return the service Cola with the highest ranking, so by default the Cola SynchStrategie is returned. The Cola packages and classes were removed, the synchronization was replaced and some potentially changes on DocShare class were made. Before make the merge with the HEAD I need to make more and more tests, so I believe that it will be enable soon. Please don't hesitate to put comments here about the changes/thoughts on DocShare.
Do we have an ETA on this?
Would it make sense for there to be an apply() method of some sorts in IModelChange or somewhere else? I found myself casting to my own interfaces / implementation classes even after having called transformRemoteChange(IModelChange).
(In reply to comment #15) > Do we have an ETA on this? > Well, I think that this bug is pretty well addressed by the sync API...i.e. it's usage in existing docshare...which Marcelo already has working on the docshare project branch. Or are there other/further things on this bug that are intended as well?
(In reply to comment #16) > Would it make sense for there to be an apply() method of some sorts in > IModelChange or somewhere else? I found myself casting to my own interfaces / > implementation classes even after having called > transformRemoteChange(IModelChange). > You are thinking it would be IModelChange.apply()...or IModelChange.apply(Object model) or something else? Seems like generally a good idea, but have to figure out the right/reasonable signature.
In reply to comment #17) > Well, I think that this bug is pretty well addressed by the sync API...i.e. > it's usage in existing docshare...which Marcelo already has working on the > docshare project branch. Sorry, I meant an ETA for a merge to HEAD. (In reply to comment #18) > You are thinking it would be IModelChange.apply()...or > IModelChange.apply(Object model) or something else? Yes. Originally I thought of apply() which was working fine for my own thing. But then as I hacked on some more code, I had a need to pass in an ID into my apply() method so I changed it to apply(ID). It makes sense for my use case but I don't know about others.
So how are developers supposed to "batch" up IModelChangeMessages? Am I supposed to create a super long byte[] from the individual byte[] arrays that are returned by serialize()? Then on the other side, deserialize the byte[] into one IModelChange, then use transformRemoteChange to split them up?
(In reply to comment #20) > So how are developers supposed to "batch" up IModelChangeMessages? Am I > supposed to create a super long byte[] from the individual byte[] arrays that > are returned by serialize()? Then on the other side, deserialize the byte[] > into one IModelChange, then use transformRemoteChange to split them up? Actually, this doesn't even make sense since ObjectInputStream's readObject() method is only going to return one message at a time...I think.
(In reply to comment #19) > In reply to comment #17) > > Well, I think that this bug is pretty well addressed by the sync API...i.e. > > it's usage in existing docshare...which Marcelo already has working on the > > docshare project branch. > > Sorry, I meant an ETA for a merge to HEAD. Just need to coordinate some with Marcelo. Can probably happen this week (Nov 24).
(In reply to comment #20) > So how are developers supposed to "batch" up IModelChangeMessages? Am I > supposed to create a super long byte[] from the individual byte[] arrays that > are returned by serialize()? Then on the other side, deserialize the byte[] > into one IModelChange, then use transformRemoteChange to split them up? > The typical case would be that there would be a 1-1 relationship between IModelChange and IModelChangeMessage (i.e. a call to IModelChangeMessage[] msgs = synchronizationStrategy.registerLocalChange(modelChange); would return a msgs array of length 1. Now, of course it could return an array longer than one...and the meaning of that would be model-type specific (as are the IModelChange instances). So depending upon the model type the meaning of multiple elements in the msgs array could mean: a) that one should do as you describe...i.e. batch the multiple messages up (i.e. wrap them as a single IModelChangeMessage), send them to remote, and deserialize them as a single IModelChange instance...for passing to transformRemoteChange b) send them as distinct/multiple IModelChangeMessage instances (i.e. in sequence)...e.g. IModelChangeMessage[] msgs = synchStrategy.registerLocalChange(modelChange); for(i=0; i < msgs.length; i++) { channel.sendMessage(target, msgs[i].serialize()); } ...and on the receiving side one would get multiple calls to the channel listener, and could then receive/apply the change instances individually. Since either/both could be correct for a given model type, I don't know that we can dictate either approach.
(In reply to comment #23) > The typical case would be that there would be a 1-1 relationship between > IModelChange and IModelChangeMessage (i.e. a call to > > IModelChangeMessage[] msgs = > synchronizationStrategy.registerLocalChange(modelChange); > > would return a msgs array of length 1. Yes, that's what I was seeing on my end too which made me question the point of returning an array. > Now, of course it could return an array longer than one...and the meaning of > that would be model-type specific (as are the IModelChange instances). But yeah, it's only 1 in my case (currently, that is) and probably isn't always 1 for other models as you noted, so returning an array seems to make sense/isn't a big deal to me. > So > depending upon the model type the meaning of multiple elements in the msgs > array could mean: > > a) that one should do as you describe...i.e. batch the multiple messages up > (i.e. wrap them as a single IModelChangeMessage), send them to remote, and > deserialize them as a single IModelChange instance...for passing to > transformRemoteChange > > b) send them as distinct/multiple IModelChangeMessage instances (i.e. in > sequence)...e.g. > > IModelChangeMessage[] msgs = synchStrategy.registerLocalChange(modelChange); > > for(i=0; i < msgs.length; i++) { > channel.sendMessage(target, msgs[i].serialize()); > } > > ...and on the receiving side one would get multiple calls to the channel > listener, and could then receive/apply the change instances individually. Yeah, I was doing B originally but then I was concerned about performance (less "filler" data to send over the network if I perform one operation instead of N operations) and potential "event storms". > Since either/both could be correct for a given model type, I don't know that we > can dictate either approach. True, good point.
Assigning bug to Marcelo. The plan for completing this bug is this: Marcelo has a little more work to do on modifications to docshare to use the new sync API (org.eclipse.ecf.sync) before committing the existing work to HEAD and Release_2_1 streams. The code on the sync_api_refactor branch has the necessary changes already, but further testing will be done, and there will be some src level code cleanup (so that it serves as some good example code for sync API usage) prior to merging back onto the Release_2_1 and HEAD streams. Marcelo will complete that work and then on Nov 25 or 26 will merge the contents of the sync_api_refactor branch (for the org.eclipse.ecf.docshare bundle) to HEAD and Release_2_1, then make a brief announcement about merge completion on this bug. Those that can, please help test the new code on sync_api_refactor branch in anticipation for this. NOTE: the new docshare is *incompatible* with the ECF 2.0.X stream, and so *both* clients have to be using the new version.
(In reply to comment #19) > (In reply to comment #18) > > You are thinking it would be IModelChange.apply()...or > > IModelChange.apply(Object model) or something else? > > Yes. Originally I thought of apply() which was working fine for my own thing. > But then as I hacked on some more code, I had a need to pass in an ID into my > apply() method so I changed it to apply(ID). It makes sense for my use case but > I don't know about others. > Remy I'm thinking of adding this method to IModelChange interface: public void applyToModel(Object model) throws ModelUpdateException; (new exception type) What do you think? The IModelChange implementer has to know the expected type of the Object model and do the downcast...for example the IDocumentChange interface would have to downcast to IDocument (which means we would have to add jface.text to sync API dependencies to provide IDocumentChange, but I don't see this as a real problem). This would make things simpler from the client's point of view...e.g: IModelChange mc = syncStrategy.transformRemoteChange(remoteChange); mc.applyToModel(document);
The work on sync_api_refactor branch was completed and tests were done. The code was merged back onto the Release_2_1 and HEAD streams.
Scott, I guess the signature for applyToModel method is interesting....let's wait Remy's opinion
(In reply to comment #26) > public void applyToModel(Object model) throws ModelUpdateException; This looks fine to me.
By the way, is this plug-in supposed to be internal or not? I ask because I'm seeing this... Export-Package: org.eclipse.ecf.docshare;x-internal:=true, org.eclipse.ecf.docshare.menu;x-internal:=true, org.eclipse.ecf.docshare.messages;x-internal:=true, org.eclipse.ecf.internal.docshare;x-internal:=true
I fixed it on HEAD and Release_2_1 streams Export-Package: org.eclipse.ecf.docshare, org.eclipse.ecf.docshare.menu, org.eclipse.ecf.docshare.messages, org.eclipse.ecf.internal.docshare;x-internal:=true
(In reply to comment #29) > (In reply to comment #26) > > public void applyToModel(Object model) throws ModelUpdateException; > > This looks fine to me. > OK thanks for comment...I've coordinated with Marcelo and I'll submit to HEAD later today 11/27.
(In reply to comment #32) > (In reply to comment #29) > > (In reply to comment #26) > > > public void applyToModel(Object model) throws ModelUpdateException; > > > > This looks fine to me. > > > > OK thanks for comment...I've coordinated with Marcelo and I'll submit to HEAD > later today 11/27. > I've commited this addition to sync API...i.e. IModelChange.applyToModel(Object model) to HEAD and Release_2_1 stream.
(In reply to comment #33) > I've commited this addition to sync API...i.e. IModelChange.applyToModel(Object > model) to HEAD and Release_2_1 stream. I think the tests weren't updated...?
(In reply to comment #34) > (In reply to comment #33) > > I've commited this addition to sync API...i.e. IModelChange.applyToModel(Object > > model) to HEAD and Release_2_1 stream. > > I think the tests weren't updated...? > Now updated for both streams. Thanks.
There one further task required before marking this bug resolved. In the original implementation of docshare using the cola work, we had code in DocShare.documentChanged to specially handle the case when a document replacement was made (existing text was deleted and then new text inserted into that place). Cola had essentially a 'hack' to first register local changes for a del then an insert operation, and send those operations to remote (docshare receiver). Now, the creation of messages to deliver to remotes is hidden behind the sync API via a single call to (in HEAD:DocShare:line 195): IModelChangeMessage changeMessages[] = syncStrategy.registerLocalChange(new DocumentChangeMessage(event.getOffset(), event.getLength(), event.getText())); for (int i = 0; i < changeMessages.length; i++) { try { sendMessage(getOtherID(), changeMessages[i].serialize()); } catch (final Exception e) { logError(Messages.DocShare_EXCEPTION_SEND_MESSAGE, e); } } Note this now calls the syncStrategy.registerLocalChange only *once* and the return value from register local change can provide one (or more) IModelChangeMessages that are actually sent to the receiver. So, the cola implementation of syncStrategy.registerLocal change now has to be updated (i.e. org.eclipse.ecf.internal.sync.doc.cola.ColaSynchronizationStrategy.registerLocalChange) so that the replace operation is properly detected and handled. It can/could do this the way we were previously doing it (i.e. by creating two messages to return ...i.e. del followed by insert), or it could introduce a new 'replace' operation to send to receivers. I have a feeling that this actually should be implemented as a single operation, so that it is atomically handled on the receiver, but need to verify with Mustafa that this is the case. Re-assigning to Mustafa, so that he can make the appropriate judgment here.
Mustafa could you please get in touch with me (Scott), when you have a chance so we can coordinate the final bit of work on this, and resolve this bug?
Created attachment 121326 [details] fix for replacement message handling This patch is meant to address the issue identified in comment #36. Mustafa and Marcelo please review this patch and give +1 here if appropriate and I will then commit to HEAD. Basically, all this does is add logic for creating and sending a del message and an ins message to substitute for a replace.
(In reply to comment #38) > Created an attachment (id=121326) [details] > fix for replacement message handling > > This patch is meant to address the issue identified in comment #36. Mustafa > and Marcelo please review this patch and give +1 here if appropriate and I will > then commit to HEAD. Basically, all this does is add logic for creating and > sending a del message and an ins message to substitute for a replace. > I've released this patch to HEAD/ECF 3.0. All tests pass. Resolving this bug as the ECF sync API now provides a separation between cola synchronization (sync API provider) and ui .
