New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Locks #301
Locks #301
Conversation
* @author Mike Durbin | ||
*/ | ||
|
||
public class LockReleasingSession implements InvocationHandler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a TODO or a link to pivotal (https://www.pivotaltracker.com/story/show/69574878) that would be helpful to indicate how/when this class can go away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done... see below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does AbstractInvocationHandler buy us in this case? equals/hashCode/toString?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, and given the need to track Sessions, I think it might be as well to have a good .equals() and .hashCode() handy. It's also going to be a bit shorter than what's here. Just a thought-- I don't think it makes a huge difference or anything like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My implementation here is similar to the other use of this pattern in our code base (TxAwareSession). I think it makes our codebase less maintainable to implement them using different conventions and different third party dependencies. I also think it's beyond the scope of this ticket to refactor other code to reflect a new best practice for dynamic proxies so I would ask that this change request be put in another ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @mikedurbin. Good point. Consistency is probably the best approach here.
public RdfStream getLock(@PathParam("path") final List<PathSegment> pathList) throws RepositoryException { | ||
|
||
final String path = toPath(pathList); | ||
Node node = session.getNode(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there is reason "node" is not "final"?
* Unlocks a lock with its token, asserts that it is successful and further | ||
* asserts that property updates can again be made without the token. | ||
*/ | ||
public void assertUnlockWithToken(String pid, String lockToken) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private
Resolved with: a766c91 Addresses: https://www.pivotaltracker.com/story/show/66093788 |
You'll want to squash these two commits since the first won't build due to rebased changes.