Community
Participate
Working Groups
Detaching objects from a view is needed, for example, when objects are moved from one transaction to another. Possibly it's also needed when transitioning objects to TRANSIENT.
I'm struggling a bit. This is more complex than I originally thought ;-( There are several use-cases (triggers) for detach: 1) Object is simply removed (or unset or cleared or overwritten by set) from a containment reference. This can also happen as an inverse operation. 2) CDOResource is removed from the resources list of the ResourceSet. This can also happen as an inverse operation. 3) An object is moved within one CDOTransaction. This leads to a remove plus an add. Should the CDOID/version of the object stay the same when "readding"??? 4) An object is moved to another CDOTransaction. It should get a new CDOID/version=1. 5) ??? Detaching means to transition an object from non-transient to transient. But this reminds me that transient is not just transient. It always means "transient with respect to a given view"!! Clearly this is not reflected in the code (other than in the test case asserts). I fear I can't implement detach without finally implementing a view-aware isTransient() method in FSMUtil. This will have major impact on the state machine and I don't know so far how to design this additional condition...
I don't see the problem. Detach mechanism and rollback are two different things.. right ? To be sure I understand correctly, Does your problem is more related to rollback or to detach ? Detach mechanism doesn't need to remember where the object come from... right ? To implement detach... it is only removing objects(CDOResource or CDOObject) from another objects change it state to transient. When you implement detach you don't need to worry about : Number 3 in your list, since this is the work of the rollback mechanism. Now, the rollback mechanism need to be more aware of the inverse operation you mentionned. When you play with two CDOTransactions, you would need to rollback both CDOTransaction... right ? In this case, both of them will react correctly.(One will add the other will remove) If you are doing only a rollback from one side ..it will only detach.. or add the objects.. the other will be in a strange state. (In reply to comment #1) > I'm struggling a bit. This is more complex than I originally thought ;-( > There are several use-cases (triggers) for detach: > 1) Object is simply removed (or unset or cleared or overwritten by set) from a > containment reference. This can also happen as an inverse operation. > 2) CDOResource is removed from the resources list of the ResourceSet. This can > also happen as an inverse operation. > 3) An object is moved within one CDOTransaction. This leads to a remove plus an > add. Should the CDOID/version of the object stay the same when "readding"??? > 4) An object is moved to another CDOTransaction. It should get a new > CDOID/version=1. > 5) ??? > Detaching means to transition an object from non-transient to transient. But > this reminds me that transient is not just transient. It always means > "transient with respect to a given view"!! Clearly this is not reflected in the > code (other than in the test case asserts). I fear I can't implement detach > without finally implementing a view-aware isTransient() method in FSMUtil. This > will have major impact on the state machine and I don't know so far how to > design this additional condition...
No, I did neither talk nor think about rollback. This seems a completely different story to me. Agreed that we have no public API for detach? It is only derived from certain other triggers in the store just like attach. But if we simply detach an object e.g. in CDOStore.remove() then we detach it also for moves from on list to another list. Detaching is a comparingly expensive operation since, if we want to do it correctly, all the data must be copied from the revision to the transient settings in the object. This can even lead to load requests in the course of dereferencing CDOIDs! Got a clue what I mean? ;-)
May be we can do the "depopulation" of the revision (copying the revision values to the transient settings of the object) lazily. Detach would then mean to set the object state to transient and leave all other fields intact (particularly view and revision). Of course the object would have to be removed from CDOViewImpl.objects as well. Then, when attaching this same object to an arbitrary view we'd have to check if it is "lazy"-transient and the new view is identical with the old view. In this case just the state is changed to DIRTY. Otherwise the revision would have to be migrated which potentially includes a different CDORevisionManager. But the second case should be rare comapred to the first one.
Hmm, but then the object would not work correctly until it gets attached to a CDOView again ;-( And there could be issues with garbage collection... I fear there is no alternative to eagerly copy the revision state into the transient area exactly at the time detach is necessary. And that can also be during view-internal moves.
(In reply to comment #5) > Hmm, but then the object would not work correctly until it gets attached to a > CDOView again ;-( And there could be issues with garbage collection... > I fear there is no alternative to eagerly copy the revision state into the > transient area exactly at the time detach is necessary. And that can also be > during view-internal moves. I agree, we do exactly the inverse of the attach.
I'm just about to write TCs for the new depopulation code ;-)
Do you still work on that bugs ? Did you start to write something?
Not at the moment. I struggled several times due to conceptual issues (see my other comments). Everything that got written is in HEAD.
Reversioned due to graduation
*** Bug 244424 has been marked as a duplicate of this bug. ***
Created attachment 110428 [details] New Design Patch created from "detach" branch against HEAD. Simon, ready to be merged into "NewContainment" when you're ready...
Created attachment 110429 [details] Use case "Attach"
Created attachment 110430 [details] Use case "Detach"
Created attachment 110431 [details] Use case "Move"
Created attachment 110432 [details] Use case "Switch"
Created attachment 110433 [details] Use case "Re-Attach"
Hi Eike, is it possible that you could change the "assigned-to" field of this bugzilla to emf.cdo-inbox@eclipse.org? I'm interested on it and realized I wasn't receiving any mailing :P I'll take a look at the branch code, I'm getting NPE while deleting an element from the object tree, I assume it's related with this bug. Best Regards, ViK.
I will start with the following: - DetachTransaction will call view.unregisterObject - CDOSavePoint will have a list of detachObjects (In the cases where a newObject is detach, it will only be removed from the newList) - When we commit, Detach object will be send to the server. - Add the following to CommitContext : public CDORevision[] getNewObjects(); THis will solve Stefan problem. Should I do that with all my others changes.. or Could I just use HEAD and provide a patch?
Please note that my problem is in fact two cases: Both ADD - REMOVE - COMMIT [object in newObject list] and ADD - COMMIT - REMOVE - COMMIT don't work. I suggest to add testcases according to Bug 244424 to test these cases.
(In reply to comment #18) > is it possible that you could change the "assigned-to" field of this bugzilla > to emf.cdo-inbox@eclipse.org? I'm interested on it and realized I wasn't > receiving any mailing :P i can't hold all bugs in the inbox. the inbox is only there so that you get notified on *new* bugs. then you can decide to cc yourself if you're further interested. for most of the existing bugs (pre-inbox "era") you'd have to go through a query and selectively cc yourself or do a mass update. > I'll take a look at the branch code, I'm getting NPE while deleting an element > from the object tree, I assume it's related with this bug. the code in the branch is far from being complete at the moment. i'll keep you informed here about the progress.
(In reply to comment #19) > I will start with the following: > - DetachTransaction will call view.unregisterObject ok. > - CDOSavePoint will have a list of detachObjects (In the cases where a > newObject is detach, it will only be removed from the newList) good. > - When we commit, Detach object will be send to the server. only the id i guess? > - Add the following to CommitContext : > public CDORevision[] getNewObjects(); what about getRemovedObjects() ? > THis will solve Stefan problem. > Should I do that with all my others changes.. or Could I just use HEAD and > provide a patch? please see the other email i sent to you ;-)
Created attachment 110520 [details] Patch for BETA 1
I'm not finish yet but all testcases are working. I added DetachTest. It test case 1, 2 and 3. Not 4 and 5 yet. I don't like the way we handle add/remove/load Resource. I think we will have the time to review it when support external will be there. It is why I didn't spend time on that problem yet. (In the current patch) I don't handle detach Resource yet. The code will slightly change.. when it comes to add & remove resources. I just changed it to make it work. I refactored CDOSavePoint to have getAllNewObjects, getAllRevisions, etc... I updated CDOSavePoint to handle removedObjects. I kept CDOStore.setContainer since I need to generate Delta. I added removedObjects to be send to the server I added FSMUtil.checkLegacySystemAvailability in CDOStore. I throw an exception when we detach an object from the View and try to access it with the id. In CDOResourceFactoryImpl, the state of CDOresource is at PROXY when isExisting == true. I don't know if it is good... But I know it will not stay like this since the way to create Resource changed in my other feature (Support external feature). What else...it is late.. I will go to bed!
Hey, as always you're the geatest!! I think cases 1-3 are the most important ones. The others are more like sugar. What you describe looks very good (but I haven't had the time to look at the patch). Regarding CDOResourceImpl being created in state PROXY I already tried hard to remember why exactly this was needed (and it was!). I think it was related to the way the Load(Resource)Transition was programmed. Maybe combined with the fact that often resource instances are locally needed (each revision has a resourceID field) but not necessarily in a loaded state. If we feel that it's no longer needed (i.e. they can start in TRANSIENT) and proof that through tests, I'd be glad to get rid of the hack.
I need to define meaningful exception and add javadoc. I'm doing that right now. I submitted the patch only to give you an idea where I was going. It is not complete yet. I hop it will be for tonight!! (for case 1,2,3).maybe 4 and 5 The patch you submitted was really helpful. Thank you! PROXY State is exactly what you thought. (for Load(Resource)Transition) If I remember correctly we will be able to remove it in the future.(Not sure at 100%)
(In reply to comment #26) > I need to define meaningful exception and add javadoc. I'm doing that right > now. > I submitted the patch only to give you an idea where I was going. It is not > complete yet. I hop it will be for tonight!! (for case 1,2,3).maybe 4 and 5 I hope that I can take the time to look at your patch tomorrow. I'm currently quite busy with the NASA slides. > The patch you submitted was really helpful. Thank you! > PROXY State is exactly what you thought. (for Load(Resource)Transition) > If I remember correctly we will be able to remove it in the future.(Not sure at > 100%) Good to see that I still have some degree of overview ;-) But more comments wil be necessary in the future!
Created attachment 110638 [details] Better patch
In HEAD, we can detach objects/resources. Did we miss some requirements ? If no I think we can close this bug. Right ?
Yes, it was committed to HEAD with bug #213402.
Fix available in HEAD: 2.0.0.I200809110653.
Generally available.
