Bugzilla – Full Text Bug Listing
| Summary: | Better aligning of CDO and EMF notifications | ||
|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Markus Barchfeld <Markus.Barchfeld> |
| Component: | cdo.core | Assignee: | Project Inbox <emf.cdo-inbox> |
| Status: | NEW --- | QA Contact: | Eike Stepper <stepper> |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | laurent.fasani, pascal.lehmann |
| Version: | 4.13 | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | odte | ||
| Bug Depends on: | |||
| Bug Blocks: | 389173 | ||
| Attachments: | |||
Created attachment 180545 [details]
Test an patch for CDO 3.0 Maintenance Branch
Hi Markus, I wanted to take a quick look at your changes but unfortunately they seem entirely out of sync with current HEAD. Can you please try provide an updated patch? Created attachment 182119 [details]
Patch for notification handling based on HEAD of 20101031
There are four tests, from which only three are green. The purpose of the remaining test (testDeliverNotificationsTwoObjects) is only to show the tough case Eike mentioned as being critical.
Hi Markus, Thank you for re-aligning the patch! Experience showed that it is unlikely that I will get to normal work during the Eclipse Summit. I'll probably look at your patch after it ;-) Some comments so far: - What is notification.getEventType() != -1, see CDONotificationBuilder.CDONotificationDispatcherVisitor.dispatch(CDOFeatureDelta) - CDO AllTests has 7 errors and 43 failures with your patch applied. Some of them are related with detachment, some with multi-session notification. I'll attach the complete failure list, but I would also like you to run the test suite yourself. - You may want to set your name as author in the new files. Please confirm that: 1) You are the only author of these changed lines. 2) You apply the EPL to these changed lines. Created attachment 183076 [details]
FailureList
Created attachment 183077 [details]
Patch v2
Just a sidenote when looking at the patch: Using CDOFeatureDelta as the key in a hashmap or in lists might have some undesired bahaviour since the implementation of hashcode/equals will lead to the same result for different CDOFeatureDeltas. Created attachment 183536 [details]
Fixed test issues, implemented testDeliverNotificationWithLists and testDeliverNotificationsTwoObjects
The latest version of the patch uses a switch on the view to enable/disable the new behavior. It has become less intrusive and the test suite (AllTests) is fine now. Except for one of the new tests for notification delivery which fails with CDO Legacy adapter. I have not looked into that yet. Pascal, I use CDOFeatureDelta in a sorted map as key. What would you suggest to avoid the "undesired behavior" you mentioned? How do we proceed? Could someone (Eike) review the patch and then decide if it is going to make it and/or what else needs to be done? I had no time yet to run our application on top of the patched CDO and check how much it has improved. I guess that I will find some more issues once I do so. However, before I spend more time on the patch I would like to know if the direction is OK. Thanks Markus I tried different workarounds such as using IdentityHashMaps, wrapper objects, using the index in case of a list instead of the CDOFeature itself or using my utility methods which were trying to use instance equal whenever possible. For all solutions using the instance to compare you get problems whenever a copy of the delta is made, or whenever a CDOMoveFeatureDelta has to be recreated (see Bug #330904). To have a general solution which works without doing any workarounds, I think we need to add something to the CDOFeatureDeltas which makes them more distinguishable without breaking the equality needed inside CDO itself. I opened another bugzilla, so we can discuss it there: https://bugs.eclipse.org/bugs/show_bug.cgi?id=330903 Markus, I was just trying to apply your patch but these two files show conflicts: CDOFeatureDeltaImpl.java CDORevisionDeltaImpl.java Can you please try to update the patch with HEAD? Sorry for the unconvenience... (In reply to comment #12) > Can you please try to update the patch with HEAD? Sorry for the > unconvenience... Hi Eike, Thanks for your answer. I think the patch worked fine when I added it here a couple of weeks ago. I guess that things have moved on in the meantime. Unfortunately, I can not care for it at the moment. With regard to the time and effort I put into the patch already I would really appreciate if you could have another look and maybe fix it. I would expect failures to be related to recent changes. Thanks Markus Hi Markus, From the hours I've already spent on your desire you can see that I appreciate your efforts very much. Unfortunately CVS patches can go out of sync and they usually do if a project is active. As you know best what you've originally changed it's your task to bring them up to date again. I hope that you understand that I can not do this. Cheers /Eike Moving all open enhancement requests to 4.1 Moving all open issues to 4.2. Open bugs can be ported to 4.1 maintenance after they've been fixed in master. Moving all outstanding enhancements to 4.3 Moving all open enhancement requests to 4.4 Moving all open enhancement requests to 4.4 Moving all open bugzillas to 4.5. Moving all unaddressed bugzillas to 4.6. Moving all open bugs to 4.7 Moving all unresolved issues to version 4.8- Moving all unresolved issues to version 4.9 Moving to 4.13. |
Build Identifier: 3.5 Let's begin with a short example to motivate this request for an enhancement: There are two CDO transactions t1 and t2 connected to the same resource, with o1 being a CDO object retrieved from t1 and o2 from t2. At least t1 must have it's change subscription policy set to CDOAdapterPolicy. session1: o1.eAdapters().add(notificationHandler1); session2: assert o2.getStreet() == null o2.eAdapters().add(notificationHandler2); o2.setName("me"); o2.setStreet("street"); o2.setName("you"); t2.commit(); In the case of o2 the adapter is called with the regular EMF mechanism for dispatching notifications. In the case of o1 CDO handles the dispatch of notifications during the process of updating the session (delivering passive updates or "session invalidation"). Only if both dispatch mechanism were completely compatible, the notification handler could be unaware of the scenario in which they are used. Unfortunately, there are some differences, which troubled us while porting an application with sophisticated notification handling to CDO. Some of the differences are: * the order of CDO notifications is random. The notification handler for o2 is called at first with the notification concerning the change of the name and after that with a notification for the change of the street. However, the order in which the handler for o1 is called is undefined. * CDO applies *all* change deltas first and later dispatches the notifications. Therefore the state of the notifier is different in both scenarios. When the handler of o2 is notified about the change of the name, o2.getStreet() will return null. However, when the handler of o1 is notified o1.getStreet() will return "street". * CDO optimizes change deltas. Therefore notificationHandler2 is called for both "me" and "you" whereas notificationHandler1 is only called for "you" I think that it would mitigate the effort to migrate applications to CDO with CDO notifications being as close as possible to EMF notifications. On the other side I think that optimization is an issue, too. So, maybe an option to switch to a "dispatch compatibility mode" would be best. Attached you find some tests and a very raw implementation which addresses the three issues. In particular the "old value" argument is not determined correctly and conflicts are ignored. I would be glad if this enhancement would be considered useful for other users of CDO, too. Reproducible: Always Steps to Reproduce: enhancement.