[GitHub] [helix] pkuwm commented on a change in pull request #1564: Implement Helix lock priority and notification

[GitBox <gi...@apache.org>]

pkuwm commented on a change in pull request #1564:
URL: https://github.com/apache/helix/pull/1564#discussion_r535859104



##########
File path: helix-lock/src/main/java/org/apache/helix/lock/helix/ZKDistributedNonblockingLock.java
##########
@@ -31,69 +31,157 @@
 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.datamodel.ZNRecordUpdater;
 import org.apache.helix.zookeeper.zkclient.DataUpdater;
-import org.apache.log4j.Logger;
+import org.apache.helix.zookeeper.zkclient.IZkDataListener;
+import org.apache.zookeeper.data.Stat;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.helix.lock.LockInfo.ZNODE_ID;
 
 
 /**
  * Helix nonblocking lock implementation based on Zookeeper.
  * NOTE: do NOT use ephemeral nodes in this implementation because ephemeral mode is not supported
  * in ZooScalability mode.
  */
-public class ZKDistributedNonblockingLock implements DistributedLock {
-  private static final Logger LOG = Logger.getLogger(ZKDistributedNonblockingLock.class);
+public class ZKDistributedNonblockingLock implements DistributedLock, IZkDataListener {
+  private static final Logger LOG = LoggerFactory.getLogger(ZKDistributedNonblockingLock.class);
 
   private final String _lockPath;
   private final String _userId;
-  private final long _timeout;
   private final String _lockMsg;
+  private final long _leaseTimeout;
+  private final long _waitingTimeout;
+  private final long _cleanupTimeout;
+  private final int _priority;
+  private final boolean _isForceful;
+  private final LockListener _lockListener;
   private final BaseDataAccessor<ZNRecord> _baseDataAccessor;
+  private boolean _isLocked;
+  private boolean _isPending;
+  private boolean _isPreempted;
+  private long _pendingTimeout;
 
   /**
    * Initialize the lock with user provided information, e.g.,cluster, scope, etc.
    * @param scope the scope to lock
    * @param zkAddress the zk address the cluster connects to
-   * @param timeout the timeout period of the lock
+   * @param leaseTimeout the leasing timeout period of the lock
    * @param lockMsg the reason for having this lock
    * @param userId a universal unique userId for lock owner identity
    */
-  public ZKDistributedNonblockingLock(LockScope scope, String zkAddress, Long timeout,
+  public ZKDistributedNonblockingLock(LockScope scope, String zkAddress, Long leaseTimeout,
       String lockMsg, String userId) {
-    this(scope.getPath(), timeout, lockMsg, userId, new ZkBaseDataAccessor<ZNRecord>(zkAddress));
+    this(scope.getPath(), leaseTimeout, lockMsg, userId, 0, Integer.MAX_VALUE, 0, false,
+        null, new ZkBaseDataAccessor<ZNRecord>(zkAddress));
+  }
+
+  /**
+   * Initialize the lock with user provided information, e.g.,cluster, scope, etc.
+   * @param scope the scope to lock
+   * @param zkAddress the zk address the cluster connects to
+   * @param leaseTimeout the leasing timeout period of the lock
+   * @param lockMsg the reason for having this lock
+   * @param userId a universal unique userId for lock owner identity
+   * @param priority the priority of the lock
+   * @param waitingTimeout the waiting timeout period of the lock when the tryLock request is issued
+   * @param cleanupTimeout the time period needed to finish the cleanup work by the lock when it
+   *                      is preempted
+   * @param isForceful whether the lock is a forceful one. This determines the behavior when the
+   *                   lock encountered an exception during preempting lower priority lock
+   * @param lockListener the listener associated to the lock
+   */
+  public ZKDistributedNonblockingLock(LockScope scope, String zkAddress, Long leaseTimeout,

Review comment:
       It seems `leaseTimeout` does not need `Long` boxing. `long` is fine?
   
   And, by seeing the list of params, just wow... So many params :)
   Can we encapsulate some of the params into an object, eg. `ZkDistributedLockProperties` or `ZkDistributedLockConfig`.




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