You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/09/25 19:35:27 UTC
[GitHub] [helix] narendly opened a new pull request #1409: Add Builder to ZKDistributedNonblockingLock to enable ZooScalability use
narendly opened a new pull request #1409:
URL: https://github.com/apache/helix/pull/1409
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1408
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
ZKDistributedNonblockingLock and its related helix-lock API were not ZooScalability-compliant, meaning they could not be used in a multi-ZK environment. This PR updates the said APIs so that we could use them in multi-ZK environment by adding a Builder that allows users to set multi-ZK parameters.
### Tests
- [ ] The following tests are written for this issue:
No logic change.
- [x] The following is the result of the "mvn test" command on the appropriate module:
helix-lock:
```
[INFO] Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.565 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 6, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
```
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1409: Add Builder to ZKDistributedNonblockingLock to enable ZooScalability use
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1409:
URL: https://github.com/apache/helix/pull/1409#discussion_r497129789
##########
File path: helix-lock/src/main/java/org/apache/helix/lock/helix/ZKDistributedNonblockingLock.java
##########
@@ -24,17 +24,21 @@
import org.apache.helix.AccessOption;
import org.apache.helix.BaseDataAccessor;
import org.apache.helix.HelixException;
+import org.apache.helix.SystemPropertyKeys;
import org.apache.helix.lock.DistributedLock;
import org.apache.helix.lock.LockInfo;
import org.apache.helix.lock.LockScope;
+import org.apache.helix.manager.zk.GenericZkHelixApiBuilder;
import org.apache.helix.manager.zk.ZkBaseDataAccessor;
import org.apache.helix.zookeeper.datamodel.ZNRecord;
import org.apache.helix.zookeeper.zkclient.DataUpdater;
import org.apache.log4j.Logger;
/**
- * Helix nonblocking lock implementation based on Zookeeper
+ * Helix nonblocking lock implementation based on Zookeeper.
+ * NOTE: do NOT use ephemeral nodes in this implementation because ephemeral mode is not supported
Review comment:
Yes, in order to keep things compatible, we shouldn't be using ephemeral nodes for the implementation of this class. If ephemeral nodes are needed, that would have to be a separate class with a different metadata store or mode of access.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on pull request #1409: Add Builder to ZKDistributedNonblockingLock to enable ZooScalability use
Posted by GitBox <gi...@apache.org>.
mgao0 commented on pull request #1409:
URL: https://github.com/apache/helix/pull/1409#issuecomment-699134667
Looks good in general. One thing to note: right now you are using the lock path that user passes in directly in the builder. But in the long run, we want to use the LockScope because we would need to support complex lock hierarchies. Is there a way you change the lock path in the builder to lock scope?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 edited a comment on pull request #1409: Add Builder to ZKDistributedNonblockingLock to enable ZooScalability use
Posted by GitBox <gi...@apache.org>.
mgao0 edited a comment on pull request #1409:
URL: https://github.com/apache/helix/pull/1409#issuecomment-699134667
Looks good in general. One thing to note: right now you are using the lock path that user passes in directly in the builder. But in the long run, we want to use the LockScope because we would need to support complex lock hierarchies. Can you change the setLockPath to setLockScope?
On a second thought, let's make both methods available and user can choose which one to use.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 edited a comment on pull request #1409: Add Builder to ZKDistributedNonblockingLock to enable ZooScalability use
Posted by GitBox <gi...@apache.org>.
mgao0 edited a comment on pull request #1409:
URL: https://github.com/apache/helix/pull/1409#issuecomment-699134667
Looks good in general. One thing to note: right now you are using the lock path that user passes in directly in the builder. But in the long run, we want to use the LockScope because we would need to support complex lock hierarchies. Can you change the setLockPath to setLockScope?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 edited a comment on pull request #1409: Add Builder to ZKDistributedNonblockingLock to enable ZooScalability use
Posted by GitBox <gi...@apache.org>.
mgao0 edited a comment on pull request #1409:
URL: https://github.com/apache/helix/pull/1409#issuecomment-699134667
Looks good in general. One thing to note: right now you are using the lock path that user passes in directly in the builder. But in the long run, we want to use the LockScope because we would need to support complex lock hierarchies. Can you change the setLockPath to setLockScope?
-------------------------------------
On a second thought, let's make both methods available and user can choose which one to use.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on pull request #1409: Add Builder to ZKDistributedNonblockingLock to enable ZooScalability use
Posted by GitBox <gi...@apache.org>.
narendly commented on pull request #1409:
URL: https://github.com/apache/helix/pull/1409#issuecomment-702893662
Add Builder to ZKDistributedNonblockingLock to enable ZooScalability use
ZKDistributedNonblockingLock and its related helix-lock API were not ZooScalability-compliant, meaning they could not be used in a multi-ZK environment. This PR updates the said APIs so that we could use them in multi-ZK environment by adding a Builder that allows users to set multi-ZK parameters.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1409: Add Builder to ZKDistributedNonblockingLock to enable ZooScalability use
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1409:
URL: https://github.com/apache/helix/pull/1409#discussion_r497073844
##########
File path: helix-lock/src/main/java/org/apache/helix/lock/helix/ZKDistributedNonblockingLock.java
##########
@@ -24,17 +24,21 @@
import org.apache.helix.AccessOption;
import org.apache.helix.BaseDataAccessor;
import org.apache.helix.HelixException;
+import org.apache.helix.SystemPropertyKeys;
import org.apache.helix.lock.DistributedLock;
import org.apache.helix.lock.LockInfo;
import org.apache.helix.lock.LockScope;
+import org.apache.helix.manager.zk.GenericZkHelixApiBuilder;
import org.apache.helix.manager.zk.ZkBaseDataAccessor;
import org.apache.helix.zookeeper.datamodel.ZNRecord;
import org.apache.helix.zookeeper.zkclient.DataUpdater;
import org.apache.log4j.Logger;
/**
- * Helix nonblocking lock implementation based on Zookeeper
+ * Helix nonblocking lock implementation based on Zookeeper.
+ * NOTE: do NOT use ephemeral nodes in this implementation because ephemeral mode is not supported
Review comment:
I'm not sure the purpose of this comment. Is this targeting whoever changes this class? what about non zooscalablity use case?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1409: Add Builder to ZKDistributedNonblockingLock to enable ZooScalability use
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1409:
URL: https://github.com/apache/helix/pull/1409#discussion_r497074334
##########
File path: helix-lock/src/main/java/org/apache/helix/lock/helix/ZKDistributedNonblockingLock.java
##########
@@ -143,11 +147,65 @@ public ZNRecord update(ZNRecord current) {
if (!(System.currentTimeMillis() < curLockInfo.getTimeout()) || isCurrentOwner()) {
return _record;
}
- // For users who are not the lock owner and try to do an update on a lock that is held by someone else, exception thrown is to be caught by data accessor, and return false for the update
+ // For users who are not the lock owner and try to do an update on a lock that is held by
+ // someone else, exception thrown is to be caught by data accessor, and return false for
+ // the update
LOG.error(
"User " + _userId + " tried to update the lock at " + new Date(System.currentTimeMillis())
+ ". Lock path: " + _lockPath);
throw new HelixException("User is not authorized to perform this operation.");
}
}
+
+ /**
+ * Builder class to use with ZKDistributedNonblockingLock.
+ */
+ public static class Builder extends GenericZkHelixApiBuilder<Builder> {
+ private LockScope _lockScope;
+ private String _userId;
+ private long _timeout;
+ private String _lockMsg;
+
+ public Builder() {
+ }
+
+ public Builder setLockScope(LockScope lockScope) {
+ _lockScope = lockScope;
+ return this;
+ }
+
+ public Builder setUserId(String userId) {
+ _userId = userId;
+ return this;
+ }
+
+ public Builder setTimeout(long timeout) {
+ _timeout = timeout;
+ return this;
+ }
+
+ public Builder setLockMsg(String lockMsg) {
+ _lockMsg = lockMsg;
+ return this;
+ }
+
+ public ZKDistributedNonblockingLock build() {
+ // Resolve which way we want to create BaseDataAccessor instance
Review comment:
This line can be put together with the following comments.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on pull request #1409: Add Builder to ZKDistributedNonblockingLock to enable ZooScalability use
Posted by GitBox <gi...@apache.org>.
narendly commented on pull request #1409:
URL: https://github.com/apache/helix/pull/1409#issuecomment-702893395
This PR is ready to be merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1409: Add Builder to ZKDistributedNonblockingLock to enable ZooScalability use
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1409:
URL: https://github.com/apache/helix/pull/1409#discussion_r497129789
##########
File path: helix-lock/src/main/java/org/apache/helix/lock/helix/ZKDistributedNonblockingLock.java
##########
@@ -24,17 +24,21 @@
import org.apache.helix.AccessOption;
import org.apache.helix.BaseDataAccessor;
import org.apache.helix.HelixException;
+import org.apache.helix.SystemPropertyKeys;
import org.apache.helix.lock.DistributedLock;
import org.apache.helix.lock.LockInfo;
import org.apache.helix.lock.LockScope;
+import org.apache.helix.manager.zk.GenericZkHelixApiBuilder;
import org.apache.helix.manager.zk.ZkBaseDataAccessor;
import org.apache.helix.zookeeper.datamodel.ZNRecord;
import org.apache.helix.zookeeper.zkclient.DataUpdater;
import org.apache.log4j.Logger;
/**
- * Helix nonblocking lock implementation based on Zookeeper
+ * Helix nonblocking lock implementation based on Zookeeper.
+ * NOTE: do NOT use ephemeral nodes in this implementation because ephemeral mode is not supported
Review comment:
Yes, in order to keep things compatible, we shouldn't be using ephemeral nodes for the implementation of this class. If ephemeral nodes are needed, that would have to be a separate class with a different metadata store or mode of access.
One can say - what about on single-zk mode? Then the behavior of this class becomes harder to understand. So it's easier to say no ephemeral node use for this class (which is the case right now).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly merged pull request #1409: Add Builder to ZKDistributedNonblockingLock to enable ZooScalability use
Posted by GitBox <gi...@apache.org>.
narendly merged pull request #1409:
URL: https://github.com/apache/helix/pull/1409
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org