[ https://jira.jboss.org/jira/browse/DNA-457?page=com.atlassian.jira.plugin... ] Brian Carothers updated DNA-457: -------------------------------- Attachment: DNA-457_WIP.patch Took a stab at locking. It's not there yet, although it does pass all of the optional locking tests (and breaks a few of the other TCK tests). In the patch: - A repository-level map of workspace names to lock managers for the workspace - A WorkspaceLockManager class that maintains an in-memory map of locked node UUIDs to locks and keeps that synchronized with a new system view at /jcr:system/dna:locks. - A new system view at /jcr:system/dna:locks that contains a child node for each live lock. - Integration between the lock methods on AbstractJcrNode and JcrSession and the WorkspaceLockManager Not in the patch: - A consistency checker that flushes some or all of the locks at startup or checks the state of /jcr:system/dna:locks against the lock information on the locked nodes - An Observer that listens for changes to the /jcr:system/dna:locks branch. This would be needed for a federated DNA install. - A locking mechanism for non-referenceable nodes. This is much more intricate than a locking approach for referenceable nodes only. - Good hygiene (e.g., meaningful comments, exception messages) Please feel free to review or provide feedback. I don't intend to finish this until after the SessionCache changes are done, as there would be a lot of interplay between the locking code and the repository logic currently in the SessionCache. -- 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-457?page=com.atlassian.jira.plugin... ] Randall Hauch commented on DNA-457: ----------------------------------- For the most part, this is a great first step! I do have a few questions, though. The general approach of persisting the lock information in /jcr:system/dna:locks seems like a good approach - it provides master persisted lock information accessible to all distributed processes. The organization seems pretty good - /jcr:system/dna:locks/<lock_for_node_given_node_uuid>, where the lock node contains the information about the node being locked as well as the information about the owner of the lock. And the restriction to mix:referenceable nodes is obvious, since the nodes' UUIDs are used in the name of the DNA lock. Considering that this approach doesn't work with nodes that are not mix:referenceable, would it work to use the UUID for the lock (rather than the UUID of the locked node) as the node name? The UUID and/or path could then be kept in the lock info node (and because this node is locked, only the session with the lock token can modify the node and possibly move the node, and in that case the session could update the path in the lock info). This would make it more difficult to quickly look up a lock given a node, but isn't that the point of the in-memory map? The in-memory map could be changed to be keyed by Location, which would work by path and/or UUID and/or by other ID properties. I'm not sure whether this would even work, because I might be missing some aspects of the current design or failing to recognize all of the issues with this alternative. Thoughts? Back to the current patch. Here are a few comments: In WorkspaceLockManager: 1) As the patch defines the code, the DnaLock.isLive() method is getting backdoor access to the map outside of a lock scope, and this seems like an error. 2) I'm not sure whether the read-write lock is guarding only the in-memory map or also the persisted content. If it's the latter, then what about other sessions created in other processes? If the read-write lock is to guard only the in-memory map, then the map probably can be changed to be a concurrent map and the read-write lock removed entirely, which could very well have a significant performance impact since this method is called once for each node in the path of a node that is being modified. Also, is the map just providing an in-memory cache of the content in the repository? 3) The lock tokens appear to just be the string form of the locked node UUID. This is pretty easily forged, correct? Not sure that this is a bad thing, but it does give users a bit more rope to hang themselves. 4) The lockFor(UUID) method could be changed to accept an AbstractJcrNode, and thus would encapsulate a bit more of the WorkspaceLockManager internals. Of course, the lockFor(String token) would need to be changed a bit to reflect how the lockFor(UUID) worked. The rest of my comments are _very_ minor nits. In AbstractJcrNode: 1) lock() will be called a lot (for every modification method on any node or property) ... seems like it'd be useful to cache some of the references. Also (minor nit) seems like the code could be cleaned up to reduce duplication by using a do-while loop. In JcrRepository: 1) JcrRepository.login(...) calls JcrRepository.getLockManager() and passes the WorkspaceLockManager reference into the JcrWorkspace constructor. This additional constructor parameter really isn't necessary, since the JcrWorkspace constructor could get it's own WorkspaceLockManager from the JcrRepository that is passed as a parameter to the constructor. 2) Some of the use of the concurrent lockManagers map is not really correct. Basically, one should never do multiple checks within a single method, since the map can change from one map method to the next. So, for example: WorkspaceLockManager getLockManager( String workspaceName ) { WorkspaceLockManager lockManager = lockManagers.get(workspaceName); if (lockManager != null) return lockManager; lockManagers.putIfAbsent(workspaceName, new WorkspaceLockManager(executionContext, this, workspaceName, locksPath)); return lockManagers.get(workspaceName); } should really be: WorkspaceLockManager getLockManager( String workspaceName ) { // Check for an existing lock manager ... WorkspaceLockManager lockManager = lockManagers.get(workspaceName); if (lockManager != null) return lockManager; // Need to create one if one hasn't just been created ... lockManager = new WorkspaceLockManager(executionContext, this, workspaceName, locksPath); WorkspaceLockManager newLockManager = lockManagers.putIfAbsent(workspaceName, lockManager); if ( newLockManager != null ) { // Some other thread just added this lock manager since our 'get' above ... lockManager = newLockManager; } return lockManager; } -- 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-457?page=com.atlassian.jira.plugin... ] Brian Carothers updated DNA-457: -------------------------------- Attachment: DNA-457_for_review_2.patch Attached new patch (DNA-457_for_review_2.patch) In WorkspaceLockManager: 1) As the patch defines the code, the DnaLock.isLive() method is getting backdoor access to the map outside of a lock scope, and this seems like an error. FIXED - switched to use of ConcurrentHashMap and removed the lock 2) I'm not sure whether the read-write lock is guarding only the in-memory map or also the persisted content. If it's the latter, then what about other sessions created in other processes? If the read-write lock is to guard only the in-memory map, then the map probably can be changed to be a concurrent map and the read-write lock removed entirely, which could very well have a significant performance impact since this method is called once for each node in the path of a node that is being modified. Also, is the map just providing an in-memory cache of the content in the repository? FIXED - Switched DnaLock to ThreadSafe (since it has no mutable internal state but does rely on the thread-safe workspace locks map), moved workspaceLocks to be a ConcurrentHashMap and deleted the manual locking 3) The lock tokens appear to just be the string form of the locked node UUID. This is pretty easily forged, correct? Not sure that this is a bad thing, but it does give users a bit more rope to hang themselves. FIXED - The lock tokens are now UUIDs themselves that are independent of the underlying node's UUID. 4) The lockFor(UUID) method could be changed to accept an AbstractJcrNode, and thus would encapsulate a bit more of the WorkspaceLockManager internals. Of course, the lockFor(String token) would need to be changed a bit to reflect how the lockFor(UUID) worked. NOT FIXED - There's a place where I need to walk up the graph from the current node to the root to make sure that none of the parents have a deep lock. I can get the underlying o.j.d.g.Node easier than the AbstractJcrNode. If I changed to using AbstractJcrNode as the argument, I'd have to do more materialization (albeit with the same number of data retrieves). I'm still open to changing this, but it doesn't seem worth the materialization penalty to me off the cuff. The rest of my comments are _very_ minor nits. In AbstractJcrNode: 1) lock() will be called a lot (for every modification method on any node or property) ... seems like it'd be useful to cache some of the references. Also (minor nit) seems like the code could be cleaned up to reduce duplication by using a do-while loop. NOT FIXED - I don't think that you can do any non-trivial lock caching (i.e., cache the results of lockFor(nodeUuid)) since that could be changed outside of the current session. I did cache the workspace().session().lockManager() calls. I think I see what you're saying about refactoring into a do-while, but the lock handling is different for a lock on the current node vs. one of its parents if lockIsDeep is false. I think it's slightly more efficient as is, but I may be missing something. In JcrRepository: 1) JcrRepository.login(...) calls JcrRepository.getLockManager() and passes the WorkspaceLockManager reference into the JcrWorkspace constructor. This additional constructor parameter really isn't necessary, since the JcrWorkspace constructor could get it's own WorkspaceLockManager from the JcrRepository that is passed as a parameter to the constructor. FIXED - Good point. The redundant constructor arg is no more. 2) Some of the use of the concurrent lockManagers map is not really correct. Basically, one should never do multiple checks within a single method, since the map can change from one map method to the next. So, for example: WorkspaceLockManager getLockManager( String workspaceName ) { WorkspaceLockManager lockManager = lockManagers.get(workspaceName); if (lockManager != null) return lockManager; lockManagers.putIfAbsent(workspaceName, new WorkspaceLockManager(executionContext, this, workspaceName, locksPath)); return lockManagers.get(workspaceName); } should really be: WorkspaceLockManager getLockManager( String workspaceName ) { // Check for an existing lock manager ... WorkspaceLockManager lockManager = lockManagers.get(workspaceName); if (lockManager != null) return lockManager; // Need to create one if one hasn't just been created ... lockManager = new WorkspaceLockManager(executionContext, this, workspaceName, locksPath); WorkspaceLockManager newLockManager = lockManagers.putIfAbsent(workspaceName, lockManager); if ( newLockManager != null ) { // Some other thread just added this lock manager since our 'get' above ... lockManager = newLockManager; } return lockManager; } FIXED - Nice catch, thanks! -- 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-457?page=com.atlassian.jira.plugin... ] Randall Hauch commented on DNA-457: ----------------------------------- Okay, I got through the rest of the patch, and it looks good. Per an IRC conversation with Brian, we agreed that WorkspaceLockManager should expose Location rather than UUID. This would make it more likely that changes to support non-referenceable nodes might be encapsulated (mostly) in WorkspaceLockManager. Other than this, the patch looks great. -- 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-457?page=com.atlassian.jira.plugin... ] Brian Carothers updated DNA-457: -------------------------------- Attachment: DNA-457_graph_locking_support.patch Attached patch that adds graph layer support for locking for review. The current approach adds two new request, LockBranchRequest and UnlockBranchRequest to communicate lock and unlock requests to the connector. The LockBranchRequest has the concept of a "scope" (currently indicates whether the lock should affect only the node or also the branch rooted at the node) and a "timeout" (the maximum TTL for the lock, 0 indicates use some default value from the connector). Any feedback would be appreciated! -- 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-457?page=com.atlassian.jira.plugin... ] Brian Carothers updated DNA-457: -------------------------------- Attachment: DNA-457_repo_locks.patch Attached patch (DNA-457_repo_locks.patch) that adds comments to address the previous feedback and provides a graph-layer API for locking nodes in repositories that provide their own locking mechanism (e.g., SVN). The patch also modifies the JCR locking code to propagate JCR locks to the graph layer. -- 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-457?page=com.atlassian.jira.plugin... ] Brian Carothers reopened DNA-457: --------------------------------- The default RequestProcessor implementation isn't correctly populating the actual location. Additionally, the RepositorySourceCapability for supports locking seems to be redundant. -- 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-457?page=com.atlassian.jira.plugin... ] Brian Carothers updated DNA-457: -------------------------------- Attachment: DNA-457_no_repo_source_capabilities.patch Added one more patch (DNA-457_no_repo_source_capabilities.patch) to remove supportsLocks() from RepositorySourceCapabilities per an IRC conversation with Randall. Coupled with the previous patch, this should re-resolve this defect. -- 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