[dna-issues] [JBoss JIRA] Commented: (DNA-457) Add JSR-170 Locking Optional Feature

[Randall Hauch (JIRA)]

    [ https://jira.jboss.org/jira/browse/DNA-457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12490206#action_12490206 ] 

Randall Hauch commented on DNA-457:
-----------------------------------

I just had a cursory look so far, but thanks for addressing my questions/suggestions.  A couple clarifications:

"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. "

Ooops.  I didn't look closely enough at the code.  Using o.j.d.g.Node would have a better suggestion, but I do see your very good point re materialization.

"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. "

Wow - another bad comment on my part.  Sorry.  All I meant by "cache some of the references" is that there were several places that "session().workspace().lockManager()" was being called within a loop, and that using a local variable would be more efficient.  The new patch uses local references.

I'll keep looking

> Add JSR-170 Locking Optional Feature
> ------------------------------------
>
>                 Key: DNA-457
>                 URL: https://jira.jboss.org/jira/browse/DNA-457
>             Project: DNA
>          Issue Type: Feature Request
>          Components: JCR
>    Affects Versions: 0.5
>            Reporter: Brian Carothers
>             Fix For: 1.0
>
>         Attachments: DNA-457_for_review.patch, DNA-457_for_review_2.patch, DNA-457_WIP.patch
>
>
> JSR-170 defines several optional features.  The locking feature allows users to lock nodes (and, optionally, their descendants) until the session terminates or until they are programatically unlocked.

-- 
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