Skip to content
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

Closed
wants to merge 5 commits into from
Closed

Locks #301

wants to merge 5 commits into from

Conversation

mikedurbin
Copy link
Contributor

You'll want to squash these two commits since the first won't build due to rebased changes.

* @author Mike Durbin
*/

public class LockReleasingSession implements InvocationHandler {
Copy link

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done... see below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

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?

Copy link
Contributor

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable.

Copy link
Contributor Author

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.

Copy link

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);
Copy link

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 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private

@awoods
Copy link

awoods commented Apr 18, 2014

Resolved with: a766c91

Addresses: https://www.pivotaltracker.com/story/show/66093788

@awoods awoods closed this Apr 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants