You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by karkum <gi...@git.apache.org> on 2014/08/16 19:15:10 UTC
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
GitHub user karkum opened a pull request:
https://github.com/apache/curator/pull/38
CURATOR-84 More flexibility for InterProcessMutex extensions
Credit goes to Jozef Vilcek for the initial patch. I just created a PR from the original patch he had created.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/karkum/curator CURATOR-84
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/38.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #38
----
commit f6c4ede47ff1cf0748d4a2418218ba8d68e4aeef
Author: Karthik Kumar <ka...@opower.com>
Date: 2014-08-16T17:11:54Z
CURATOR-84 More flexibility for InterProcessMutex extensions
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by karkum <gi...@git.apache.org>.
Github user karkum commented on a diff in the pull request:
https://github.com/apache/curator/pull/38#discussion_r16354351
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/InterProcessMutex.java ---
@@ -61,7 +61,17 @@ private LockData(Thread owningThread, String lockPath)
*/
public InterProcessMutex(CuratorFramework client, String path)
{
- this(client, path, LOCK_NAME, 1, new StandardLockInternalsDriver());
+ this(client, path, new StandardLockInternalsDriver());
+ }
+
+ /**
+ * @param client client
+ * @param path the path to lock
+ * @param driver lock driver
+ */
+ private InterProcessMutex(CuratorFramework client, String path, LockInternalsDriver driver)
--- End diff --
Ah gotcha, yes, I've updated that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/38#discussion_r16353274
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/InterProcessMutex.java ---
@@ -61,7 +61,17 @@ private LockData(Thread owningThread, String lockPath)
*/
public InterProcessMutex(CuratorFramework client, String path)
{
- this(client, path, LOCK_NAME, 1, new StandardLockInternalsDriver());
+ this(client, path, new StandardLockInternalsDriver());
+ }
+
+ /**
+ * @param client client
+ * @param path the path to lock
+ * @param driver lock driver
+ */
+ private InterProcessMutex(CuratorFramework client, String path, LockInternalsDriver driver)
--- End diff --
Sorry for the previous comment, but this, actually, needs to be public right? Also LockInternalsDriver would need to be public? The issue seeks to have nodes created with PERMANENT. Right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by karkum <gi...@git.apache.org>.
Github user karkum commented on the pull request:
https://github.com/apache/curator/pull/38#issuecomment-52633291
Ok, I've added a getter for the lockPath, let me know if this is what you were imagining. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by karkum <gi...@git.apache.org>.
Github user karkum commented on the pull request:
https://github.com/apache/curator/pull/38#issuecomment-52768395
@cammckenzie, I've added a unit test. Could you please take a look? Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by karkum <gi...@git.apache.org>.
Github user karkum commented on the pull request:
https://github.com/apache/curator/pull/38#issuecomment-53421096
@Randgalt, have you had a chance to look over this again?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/38#discussion_r16354016
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/InterProcessMutex.java ---
@@ -61,7 +61,17 @@ private LockData(Thread owningThread, String lockPath)
*/
public InterProcessMutex(CuratorFramework client, String path)
{
- this(client, path, LOCK_NAME, 1, new StandardLockInternalsDriver());
+ this(client, path, new StandardLockInternalsDriver());
+ }
+
+ /**
+ * @param client client
+ * @param path the path to lock
+ * @param driver lock driver
+ */
+ private InterProcessMutex(CuratorFramework client, String path, LockInternalsDriver driver)
--- End diff --
If you make StandardLockInternalsDriver public, LockInternalsDriver should also be public. LockInternalsDriver is the actual interface. StandardLockInternalsDriver is just one implementation of it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/38
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by jojovilco <gi...@git.apache.org>.
Github user jojovilco commented on the pull request:
https://github.com/apache/curator/pull/38#issuecomment-52509901
Wow, thanks a lot for pushing this forward!
I planed to put an enhancement into original path, but got stuck up with other things.
In InterProcessMutext, I would love to have a (public / protected) method which can returned a path to an actually owned lock znode (LockData.lockPath) of a current thread. This would enable to build a pattern over a lock which could guard for the case of lost lock and initiate an abort of currently running task.
What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/38#issuecomment-52849555
Thanks, looks good to me. I've fixed up the formatting of the unit test to match the Curator standards. Other than that, it's good to merge from my point of view. @Randgalt, any further comments?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/38#discussion_r16331213
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/InterProcessMutex.java ---
@@ -61,7 +61,17 @@ private LockData(Thread owningThread, String lockPath)
*/
public InterProcessMutex(CuratorFramework client, String path)
{
- this(client, path, LOCK_NAME, 1, new StandardLockInternalsDriver());
+ this(client, path, new StandardLockInternalsDriver());
+ }
+
+ /**
+ * @param client client
+ * @param path the path to lock
+ * @param driver lock driver
+ */
+ protected InterProcessMutex(CuratorFramework client, String path, LockInternalsDriver driver)
--- End diff --
This can be private. Is there a reason to make it protected?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by karkum <gi...@git.apache.org>.
Github user karkum commented on a diff in the pull request:
https://github.com/apache/curator/pull/38#discussion_r16353602
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/InterProcessMutex.java ---
@@ -61,7 +61,17 @@ private LockData(Thread owningThread, String lockPath)
*/
public InterProcessMutex(CuratorFramework client, String path)
{
- this(client, path, LOCK_NAME, 1, new StandardLockInternalsDriver());
+ this(client, path, new StandardLockInternalsDriver());
+ }
+
+ /**
+ * @param client client
+ * @param path the path to lock
+ * @param driver lock driver
+ */
+ private InterProcessMutex(CuratorFramework client, String path, LockInternalsDriver driver)
--- End diff --
Yes, I was just about to comment back. The plan was for a client using the ```InterProcessMutex``` to specify their own ```LockInternalsDriver``` with different parameters than the one provided by the default ```StandardLockInternalsDriver```. We definitely need this to be public.
I'm not sure what you mean by "Also LockInternalsDriver would need to be public?". Could you please elaborate?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by karkum <gi...@git.apache.org>.
Github user karkum commented on a diff in the pull request:
https://github.com/apache/curator/pull/38#discussion_r16352585
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/InterProcessMutex.java ---
@@ -61,7 +61,17 @@ private LockData(Thread owningThread, String lockPath)
*/
public InterProcessMutex(CuratorFramework client, String path)
{
- this(client, path, LOCK_NAME, 1, new StandardLockInternalsDriver());
+ this(client, path, new StandardLockInternalsDriver());
+ }
+
+ /**
+ * @param client client
+ * @param path the path to lock
+ * @param driver lock driver
+ */
+ protected InterProcessMutex(CuratorFramework client, String path, LockInternalsDriver driver)
--- End diff --
Nope changed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by jojovilco <gi...@git.apache.org>.
Github user jojovilco commented on the pull request:
https://github.com/apache/curator/pull/38#issuecomment-52597742
Yes, you are correct.
Indeed in default lock implementation it can be done by watching connection. But not in other custom lock implementations which this issue aims to target. I would make access to lockPath protected, to be managed by lock implementation itself. Public access can mislead the purpose. E.g. is it ok to directly delete the path to release the lock? No! There is different API for that.
For me, it is more intuitive to reason about lost lock from watching the actual lockPath node, than derive its existence from lost connection. But this is, off course, subjective.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by karkum <gi...@git.apache.org>.
Github user karkum commented on the pull request:
https://github.com/apache/curator/pull/38#issuecomment-52567036
Hmm, let me make sure I understand:
* In ```InterProcessMutex```, we expose the actual lockPath being used by the lock (which could be either ephemeral or persistent).
* This would allow any user of ```InterProcessMutex``` to take care of clean up/notification steps based on whether the lock of that path actually exists or not.
I think this makes sense, but I'm not sure if it is ideal to expose the actual lock path used by this recipe. What if a client is using the default implementation (with ephemeral nodes), then getting the path would be pretty useless, since that node would be deleted once connection is lost right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/38#issuecomment-52741522
Fix looks ok to me, but I think that we need some unit tests before merging. Would be nice to have a test for the case that this ticket was raised for. i.e. Creating a persistent lock node.
The existing tests are all passing though, which is good.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-84 More flexibility for InterProcess...
Posted by jojovilco <gi...@git.apache.org>.
Github user jojovilco commented on the pull request:
https://github.com/apache/curator/pull/38#issuecomment-52648702
Yes! Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---