Community
Participate
Working Groups
Build Identifier: In a test case I have the following code: data1.getHeartbeatMap().put("AAR1", heartbeat); data1.getHeartbeatMap().remove("AAR1"); commit(); CDO generates the following feature deltas: [CDORevisionDelta[ScenarioData@OID298:0v1 --> [CDOFeatureDelta[heartbeatMap, LIST, list=[CDOFeatureDelta[heartbeatMap, ADD, value=NULL, index=0], CDOFeatureDelta[heartbeatMap, REMOVE, value=UNKNOWN, index=0]]]]]] Notice the "value=NULL" for the ADD. Is this a bug in CDO or the expected behaviour? I think (I've not tested) that in this case, 2 notifications whould be generated, one ADD with null and one REMOVE. I would have expected either ADD with a value (e.g. heartbeat in this case) or no notification at all. Moreover, when doing this: data1.getHeartbeatMap().put("AAR1", heartbeat); data1.getHeartbeatMap().remove("AAR1"); data1.getHeartbeatMap().put("AAR1", heartbeat); data1.getHeartbeatMap().remove("AAR1"); data1.getHeartbeatMap().put("AAR1", heartbeat); data1.getHeartbeatMap().remove("AAR1"); data1.getHeartbeatMap().put("AAR1", heartbeat); data1.getHeartbeatMap().remove("AAR1"); data1.getHeartbeatMap().put("AAR1", heartbeat); data1.getHeartbeatMap().remove("AAR1"); data1.getHeartbeatMap().put("AAR1", heartbeat); data1.getHeartbeatMap().remove("AAR1"); commit(); CDO generates the following feature deltas: [CDORevisionDelta[ScenarioData@OID1586:0v1 --> [CDOFeatureDelta[heartbeatMap, LIST, list=[CDOFeatureDelta[heartbeatMap, ADD, value=NULL, index=0], CDOFeatureDelta[heartbeatMap, REMOVE, value=UNKNOWN, index=0], CDOFeatureDelta[heartbeatMap, ADD, value=NULL, index=0], CDOFeatureDelta[heartbeatMap, REMOVE, value=UNKNOWN, index=0], CDOFeatureDelta[heartbeatMap, ADD, value=NULL, index=0], CDOFeatureDelta[heartbeatMap, REMOVE, value=UNKNOWN, index=0], CDOFeatureDelta[heartbeatMap, ADD, value=NULL, index=0], CDOFeatureDelta[heartbeatMap, REMOVE, value=UNKNOWN, index=0], CDOFeatureDelta[heartbeatMap, ADD, value=NULL, index=0], CDOFeatureDelta[heartbeatMap, REMOVE, value=UNKNOWN, index=0], CDOFeatureDelta[heartbeatMap, ADD, value=NULL, index=0], CDOFeatureDelta[heartbeatMap, REMOVE, value=UNKNOWN, index=0]]]]]] Can't CDO optimize this? How is this handled at the database level? Reproducible: Always
Created attachment 166164 [details] CDOAddFeatureDelta with null value and redundant CDOFeatureDeltas
(In reply to comment #1) > Created an attachment (id=166164) [details] > CDOAddFeatureDelta with null value and redundant CDOFeatureDeltas This is a test case that reproduce the issue.
The NULL value in the CDOAddFeatureDelta is caused by: cachedSources[i].clear(); in CDOListFeatureDeltaImpl.cleanupWithNewDelta(CDOFeatureDelta). This happens when the CDORemoveFeatureDelta is added to the CDOListFeatureDelta. What is the purpose of CDOListFeatureDeltaImpl.cleanupWithNewDelta(CDOFeatureDelta)?
Unfortunately I have no clue at the moment. IIRC all this has been added by Simon (CC'ed). Simon has been very quiet in the recent past. If he doesn't see this question I'll try to forward it via email. Or we'll have to find out without his help ;-(
The explanation is here: https://bugs.eclipse.org/bugs/show_bug.cgi?id=250757#c3 It seems however that data binding (and others) does not really like this null value. !ENTRY org.eclipse.core.databinding 4 0 2010-05-28 15:18:37.462 !MESSAGE Unhandled exception: null argument:element cannot be null !STACK 0 org.eclipse.core.runtime.AssertionFailedException: null argument:element cannot be null at org.eclipse.core.runtime.Assert.isNotNull(Assert.java:85) at org.eclipse.jface.internal.databinding.viewers.ObservableCollectionTreeContentProvider$TreeNode.<init>(ObservableCollectionTreeContentProvider.java:410) at org.eclipse.jface.internal.databinding.viewers.ObservableCollectionTreeContentProvider.getOrCreateNode(ObservableCollectionTreeContentProvider.java:243) at org.eclipse.jface.internal.databinding.viewers.ObservableCollectionTreeContentProvider.getOrCreateNode(ObservableCollectionTreeContentProvider.java:237) at org.eclipse.jface.databinding.viewers.ObservableListTreeContentProvider$Impl.access$5(ObservableListTreeContentProvider.java:1) at org.eclipse.jface.databinding.viewers.ObservableListTreeContentProvider$Impl$ListChangeListener.handleListChange(ObservableListTreeContentProvider.java:114) at org.eclipse.core.databinding.observable.list.ListChangeEvent.dispatch(ListChangeEvent.java:61) at org.eclipse.core.databinding.observable.ChangeManager.fireEvent(ChangeManager.java:119) at org.eclipse.core.databinding.observable.list.DecoratingObservableList.fireListChange(DecoratingObservableList.java:59) at org.eclipse.core.databinding.observable.list.DecoratingObservableList.handleListChange(DecoratingObservableList.java:97) at org.eclipse.core.databinding.observable.list.DecoratingObservableList$1.handleListChange(DecoratingObservableList.java:71) at org.eclipse.core.databinding.observable.list.ListChangeEvent.dispatch(ListChangeEvent.java:61) at org.eclipse.core.databinding.observable.ChangeManager.fireEvent(ChangeManager.java:119) ... Are there any means to detect that the CDOAddFeatureDelta value has been "artificially" set to null?
I can confirn Cyril's statement in comment 5. Databinding has real problem when notifications have null values (e.g. added value is null or old value of removed object is null). The whole databinding chain will break and the UI will not display the current status correctly. See also https://bugs.eclipse.org/bugs/show_bug.cgi?id=303868
I'm working on this issue.
Thank you, Cyril. Let me know if I can help!
I tried to modify CDOListFeatureDeltaImpl so as to completely remove the ADD/REMOVE deltas from the list instead of setting the value to CDOID.NULL. But CDOListFeatureDelta may also contain CDOMoveFeatureDelta which makes the whole thing a lot more complicated (patch the indexes). Are ADD, REMOVE and MOVE the only feature deltas that may be added to a CDOListFeatureDelta? Eike, do you think that "optimizing" the CDOListFeatureDelta's feature list (by removing/modifying the feature deltas) is the right solution? This would solve the ADD with value=NULL issue and decrease the number of notifications, DB accesses, etc at the same time. Do you see a better solution?
(In reply to comment #9) > I tried to modify CDOListFeatureDeltaImpl so as to completely remove the > ADD/REMOVE deltas from the list instead of setting the value to CDOID.NULL. But > CDOListFeatureDelta may also contain CDOMoveFeatureDelta which makes the whole > thing a lot more complicated (patch the indexes). We can talk about this tomorrow, I had to do this a couple of times already ;) > Are ADD, REMOVE and MOVE the only feature deltas that may be added to a > CDOListFeatureDelta? Afaik CLEAR will be the 4th list feature delta which can occur. > Eike, do you think that "optimizing" the CDOListFeatureDelta's feature list (by > removing/modifying the feature deltas) is the right solution? This would solve > the ADD with value=NULL issue and decrease the number of notifications, DB > accesses, etc at the same time. It would also solve the IndexOutOfBoundsException in the latest testcase patch for bug #306710
(In reply to comment #10) > (In reply to comment #9) > > I tried to modify CDOListFeatureDeltaImpl so as to completely remove the > > ADD/REMOVE deltas from the list instead of setting the value to CDOID.NULL. But > > CDOListFeatureDelta may also contain CDOMoveFeatureDelta which makes the whole > > thing a lot more complicated (patch the indexes). > We can talk about this tomorrow, I had to do this a couple of times already ;) > Good idea ;-) > > Are ADD, REMOVE and MOVE the only feature deltas that may be added to a > > CDOListFeatureDelta? > Afaik CLEAR will be the 4th list feature delta which can occur. > Indeed CLEAR may be added to LIST. But it's not really a problem because CLEAR clears the feature delta list before being added to CDOListFeatureDelta. In CDORevisionDeltaImpl: // Remove all previous changes if (delta instanceof CDOClearFeatureDelta || delta instanceof CDOUnsetFeatureDelta) { listDelta.getListChanges().clear(); } listDelta.add(delta);
(In reply to comment #10) > We can talk about this tomorrow, I had to do this a couple of times already ;) So far all your assumption here looked correct. Maybe we can have a conf call altogether tomorrow.
Created attachment 171708 [details] Remove CDO[Add|Set]FeatureDelta from CDOListFeatureDelta instead of setting the value to CDOID.NULL
Out of the 4265 tests, one "randomly" gives an error [1] but I don't think it is related to this patch. If I run the test alone, it works. Probably a concurrency/timing issue. The patch removes the CDOAddFeatureDelta (or CDOSetFeatureDelta) from the CDOListFeatureDelta when a corresponding CDORemoveFeatureDelta is added. CDOFeatureDelta in the list are patched (or removed) if needed/possible during the process. 1) The number of lines that you added/changed is smaller than 250. Without comments and tests yes. With them I would say no. So feel free to remove the comments :-D 2) You are the only author of these changed lines. Yes with the help of Pascal Lehmann 3) You apply the EPL to these changed lines. Yes Hope I did not mess up too many things ;-) [1] BranchingTest.testDetachExisting [Combined, MEMBranches, JVM, Legacy]
Committed to HEAD
Available in 3.0 GA: http://download.eclipse.org/modeling/emf/cdo/updates/3.0-releases/
The committed fix is not 100% right and fails in certain cases that involve CDOSetFeatureDelta. Currently, when removing CDOSetFeatureDelta, the indexes of the other feature deltas are patched. This should not be the case because CDOSetFeatureDelta does not modify the length of the list.
Created attachment 180763 [details] Do not patch the indexes when removing a CDOSetFeatureDelta This patch also contains a fix to CDOLegacyWrapper (find out the right index instead of using 0) and a test case. 1) The number of lines that you added/changed is smaller than 250. Yes 2) You are the only author of these changed lines. Yes 3) You apply the EPL to these changed lines. Yes
Martin, here is a small change to CDOLegacyWrapper. Does it conflict with your fix in bug 327604?
Good catch, Cyril. THan k you ;-) Committed to HEAD
Moving all open problem reports to 4.0
Closing.
