Bugzilla – Full Text Bug Listing
| Summary: | Allow NEW objects to be locked on commit | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Caspar D. <caspar_d> | ||||||||||||||||||||||||||
| Component: | cdo.core | Assignee: | Caspar D. <caspar_d> | ||||||||||||||||||||||||||
| Status: | CLOSED DUPLICATE | QA Contact: | Eike Stepper <stepper> | ||||||||||||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||||||||||||
| Priority: | P3 | CC: | esteban.dugueperoux, saulius.tvarijonas, stepper | ||||||||||||||||||||||||||
| Version: | 4.6 | Flags: | stepper:
review+
|
||||||||||||||||||||||||||
| Target Milestone: | --- | ||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
|
Description
Caspar D.
Created attachment 203833 [details]
Patch v1
A few subtleties to consider: - When notifying other sessions, the commitNotification should be sent before the lockNotification, otherwise the lockNotification will reference new objects that those other sessions haven't yet been informed about. (Ideally the data and lock notifications should be combined into one signal in such situations, but perhaps that's best undertaken as a separate effort.) - When autoReleaseLocks is enabled, what should happen when a session commits a new object with a lock? Nothing, I suppose: he locks and unlocks the object with the same operation, the net effect being no change on the server. On the client however, the lock still needs to be removed from the new object's lockState. Created attachment 204046 [details]
Patch v2
Created attachment 204054 [details]
Patch v3
Created attachment 204065 [details]
Patch v4
Created attachment 204169 [details]
Patch v5
Turned out more complicated than I expected. Still, seems to work. Created attachment 204170 [details]
Patch v6
(In reply to comment #2) > - When notifying other sessions, the commitNotification should > be sent before the lockNotification, otherwise the lockNotification > will reference new objects that those other sessions haven't yet > been informed about. (Ideally the data and lock notifications > should be combined into one signal in such situations, but perhaps > that's best undertaken as a separate effort.) It should be a separate effort but we shouldn't delay it unnecessarily. I think even if the lock notification is sent after the commit notification it could easily overtake the latter on the wire. Or do the lock notifications participate in the outOfSequence ordering of session invalidations? > - When autoReleaseLocks is enabled, what should happen when a session > commits a new object with a lock? Nothing, I suppose: he locks and > unlocks the object with the same operation, the net effect being > no change on the server. On the client however, the lock still needs > to be removed from the new object's lockState. Doesn't that apply to all lock states after commit with autoReleaseLocks? Created attachment 204243 [details]
Patch v7
I've refactored (removed) CDOObjectWrapper.createCDOLock(). Same for CDOObjectWrapper.cdoLockState().
CDODataOutputImpl.writeCDOCommitData() and CDODataInputImpl.readCDOCommitData() do not consider locksOnNewObjects. That needs clarification before I can approve the review.
Created attachment 204276 [details]
Patch v8
Created attachment 204656 [details]
Patch v9
Reworked as per your proposal: the locksOnNewObjects collection is
now gettable from the CDOCommitContext iface, instead of the
CDOCommitData iface. Methods on CDOSessionProtocol adjusted to
receive this collection as an argument.
Created attachment 204658 [details]
Patch v10
Offline broken.
Created attachment 204659 [details]
Patch v11
Offline fixed.
Created attachment 204729 [details]
Patch v12
- I've removed the getLocksOnNewObjects() method from all (2) CDOCommitData implementations
- I've replaced the implementation of the deprecated methods in CDOClientProtocol by UnsupportedOperationException
- I've replaced the UnsupportedOperationException in CDOCommitContextImpl.getUserID() with return transaction.getSession().getUserID()
- I've moved the new methods from InternalCDOCommitContext to CDOCommitContext
- Throwing UnsupportedOperationException from CommitDelegationRequest.getObjectType(CDOID)
One failure in suite "H2 All" (all scenarios):
junit.framework.AssertionFailedError: expected:<true> but was:<false>
at org.eclipse.emf.cdo.tests.LockingManagerRestartTransactionTest.testDurableViewHandler(LockingManagerRestartTransactionTest.java:391)
I'm approving the patch but please look at (fix) the failing test case ;-) Committed revision 9469. Concerning the test failure: the problem is that ConfigTest.restartRepository doesn't really restart the repository, but replaces it. The RepoConfig puts a listener on every repo that gets registered (see RepositoryConfig. registerRepository), and this listener removes the repo from the config's map of repositories. The subsequent getRepo(String) name call, triggers instantation of a new repository, but of course the listener that was added by the TEST logic (not to be confused with the listener I just mentioned), is still attached to (only) the original repo, which was deactivated and discared. Possible solutions: 1. Disable this test for repository restarts 2. Fix the test logic so that restartRepo *restarts* a repo. This might be difficult. Of course the logic in the handler can be made conditional on isRestarting() -- that's trivial. But I don't think repos and all their constituent parts are prepared for being restarted. The assumption seems to have been that after a #deactivate call, the objects will not be used again. I'm not sure how much work it will be to address this. Hi Caspar, Could I have your advice about http://www.eclipse.org/forums/index.php/t/368113/ ? We have a similar need to this one where we have issue on other users lock/commit notifications reception ordering. Moving all open issues to 4.2. Open bugs can be ported to 4.1 maintenance after they've been fixed in master. Working on https://bugs.eclipse.org/bugs/show_bug.cgi?id=387563 to generalize this feature, I see that the recursive parameter is not taken into account, because If I attach a new object containment tree and I choose to lock this object containment tree with recursive parameter at true, after commit only the root of this object containment tree will be locked. I will fix that iterating with eAllContents() to register all objects to lock/unlock on commit. There is also the case where we lock recursively a object tree having objects with clean state except for a subtree for which objects are in NEW state. In this case only the objects in CLEAN state are locked and not the objects in NEW state, event after a commit. In conclusion the feature of recursive locking (CDOSessionProtocol.lockObjects2() https://bugs.eclipse.org/bugs/show_bug.cgi?id=354454 ) is not compatible with this feature of lock new objects on commit. To fix that we must iterate on the tree to see if there is object in NEW state. Then the CDOSessionProtocol.lockObjects2() operation become useless. I will manage this use case in https://bugs.eclipse.org/bugs/show_bug.cgi?id=387563 . 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. *** This bug has been marked as a duplicate of bug 387563 *** Closing. Closing. |