[ https://jira.jboss.org/jira/browse/DNA-456?page=com.atlassian.jira.plugin... ] Dan Florian commented on DNA-456: --------------------------------- Below are the TCK tests that are failing using my (not currently checked-in) implementation of the ObservationManager and why I think they are failing. One design note: I had to implement a check that makes sure Changes with a timestamp before the time a listener was registered do not get delivered to that listener. This was a problem because the events were created and the listener was being registered in one thread and the event notification was being handled in a different thread. I will write Jiras for (1) requests don't differentiate between setting a new property or changing an existing property, and (2) Changes need to include a session identifier so that the ObservationManager can handle the "noLocal" flag. ===================== LockingTest testAddLockToNode - no PROPERTY_ADDED events received testRemoveLockFromNode - no PROPERTY_REMOVED events received NodeRemovedTest testMultiNodesRemoved - no NODE_REMOVED event for child nodes when a parent is removed NodeMovedTest testMoveWithRemove - fails with NPE (originalParent) in GraphSession.containsChangesWithExternalDependencies() NodeReorderTest testNodeReorder - no events received (need to research what happens when node.orderBefore(sourcePath, destinationPath) is called) PropertyAddedTest testSinglePropertyAdded - no events received since PROPERTY_ADDED changes are implemented as PROPERTY_CHANGED events testMultiPropertyAdded - no events received since PROPERTY_ADDED changes are implemented as PROPERTY_CHANGED events PropertyChangedTest testSinglePropertyChangedWithAdded - too many events as PROPERTY_ADDED changes are being broadcast as PROPERTY_CHANGED events AddEventListenerTest testNodeType - path is /testroot/node2/node3 but should be /testroot/node1/node3 (need to research this one) testPath - should not have path /testroot/node1 (so does isDeep mean not to include parent path?) testIsDeepFalseNodeAdded - path is /testroot/node1 and should be /testroot/node1/node2 (expected test results seems wrong??) testIsDeepFalsePropertyAdded - no events since PROPERTY_ADDED changes are sent as PROPERTY_CHANGED events testNoLocalTrue - still have to implement "noLocal" check in ObservationManager testUUID - no events received since no PROPERTY_ADDED changes are implemented as PROPERTY_CHANGED events WorkspaceOperationTest testCopy - no NODE_ADDED event generated for child node of copied parent node -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://jira.jboss.org/jira/browse/DNA-456?page=com.atlassian.jira.plugin... ] Dan Florian updated DNA-456: ---------------------------- Attachment: DNA-456-dna-graph.patch DNA-456-dna-jcr.patch DNA-456-dna-repository.patch DNA-456-dna-graph.patch, DNA-456-dna-jcr.path, and DNA-456-dna-repository should all be attached together into there appropriate project. -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://jira.jboss.org/jira/browse/DNA-456?page=com.atlassian.jira.plugin... ] Randall Hauch updated DNA-456: ------------------------------ Attachment: DNA-456-fix-concurrency-issues.patch I've attached a patch that fixes all of the problems, so now all of the Observation tests should be passing (except for the still missing deep events upon delete and clone). The major problem was in how the JcrRepository.RepositoryObservationManager.notify(...) method was behaving. This Observer is actually listening to the RepositoryLibrary's ObservationBus instance, which is the observer for all of the RepositorySource objects, and the 'notify(...)' method is being called within the same thread as the RepositorySource is processing the requests (and firing the Changes). The whole purpose of the JcrRepository.RepositoryObservationManager is to quickly enqueue the Changes for processing in another thread, to get off of the thread used by the connector (so the request processing can be completed). It turns out that the 'notify(...)' method was passing the list of session observers into the Runnable. BUT THIS LIST CHANGES WHENEVER A NEW JCR LISTENER IS ADDED OR REMOVED. As a result, the changes may get sent to listeners that were registered AFTER the save(). This is the whole reason why the timestamp checking (originally) and ObservedId (more recently) were needed. The fix was pretty simple. Rather than pass the list of session observers to the Runnable, the RepositoryObservationManager should grab a snapshot of the list. This is trivial to do, since the list is a CopyOnWriteArrayList: simply get an Iterator<Observer> from the list and pass THAT to the Runnable. Thus, the snapshot of the observer list is made within the same thread where the connector is running, which is within the context of the save(). Any listener added after the save completes will not be in the snapshot used to fire off the changes. In other words, instead of this: final List<Observer> observersList = observers; Runnable sender = new Runnable() { public void run() { for (Observer observer: observersList ) { observer.notify(changes); } } }; this.observerService.execute(sender); the method should be this: final Iterator<Observer> observerIterator = observers.iterator(); Runnable sender = new Runnable() { public void run() { while (observerIterator.hasNext()) { Observer observer = observerIterator.next(); observer.notify(changes); } } }; this.observerService.execute(sender); Another improvement was added to the 'notify(...)' method. Since the JcrRepository only cares about one source (usually the federated source), it can safely ignore all Changes objects that are from other sources. Doing this saves work being added to the worker queue. After this change was made, there was no more need for the ObservedId, so that (and all uses of it) was removed. One minor problem was that SetPropertyRequest.setNewProperty(boolean) was always setting the 'is new' property to true, rather than to the supplied value. This was always resulting in a PROPERTY_ADDED event rather than a PROPERTY_CHANGED when the property is not new. Another issue was that the Federated Connector was not always firing the events in the correct order. I don't think this mattered in the results, but this was fixed. Finally, the tests in the sequencer examples were failing because the SequencingService was processing the NetChange.getModifiedProperties(). Obviously this missed any ADDED property. Therefore, a 'getAddOrModifiedProperties()' method was added to the NetChange class, and the SequencingService changed to use this method. These changes were committed, and the recently ignored tests were added back in. -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://jira.jboss.org/jira/browse/DNA-456?page=com.atlassian.jira.plugin... ] Randall Hauch commented on DNA-456: ----------------------------------- Just got a clean build, so I'm hoping this fix actually worked. Let's hope that we don't see these failures pop up anymore. Meanwhile, this issue still needs a resolution for DNA-563. -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira