You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@helix.apache.org by jx...@apache.org on 2020/04/23 20:02:52 UTC
[helix] 17/20: Clean up code
This is an automated email from the ASF dual-hosted git repository.
jxue pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/helix.git
commit 53ca68d74bcd124013fa16398885c3d4a660ddc4
Author: Molly Gao <mg...@mgao-mn1.linkedin.biz>
AuthorDate: Fri Feb 14 18:51:48 2020 -0800
Clean up code
---
.../main/java/org/apache/helix/lock/HelixLock.java | 8 +-
.../main/java/org/apache/helix/lock/LockInfo.java | 93 ++++-------------
.../main/java/org/apache/helix/lock/LockScope.java | 7 ++
.../apache/helix/lock/helix/HelixLockScope.java | 115 +++------------------
.../helix/lock/helix/ZKHelixNonblockingLock.java | 63 ++---------
.../lock/helix/TestZKHelixNonblockingLock.java | 35 +++----
6 files changed, 74 insertions(+), 247 deletions(-)
diff --git a/helix-lock/src/main/java/org/apache/helix/lock/HelixLock.java b/helix-lock/src/main/java/org/apache/helix/lock/HelixLock.java
index 1cd9e60..97e2c35 100644
--- a/helix-lock/src/main/java/org/apache/helix/lock/HelixLock.java
+++ b/helix-lock/src/main/java/org/apache/helix/lock/HelixLock.java
@@ -38,15 +38,15 @@ public interface HelixLock {
boolean releaseLock();
/**
- * Retrieve the lock information, e.g. lock timeout, lock message, etc.
+ * Retrieve the information of the current lock on the resource this lock object specifies, e.g. lock timeout, lock message, etc.
* @return lock metadata information
*/
- LockInfo getLockInfo();
+ LockInfo getCurrentLockInfo();
/**
- * If the user is current lock owner
+ * If the user is current lock owner of the resource
* @return true if the user is the lock owner,
* false if the user is not the lock owner or the lock doesn't have a owner
*/
- boolean isOwner();
+ boolean isCurrentOwner();
}
diff --git a/helix-lock/src/main/java/org/apache/helix/lock/LockInfo.java b/helix-lock/src/main/java/org/apache/helix/lock/LockInfo.java
index 212c93c..250fb71 100644
--- a/helix-lock/src/main/java/org/apache/helix/lock/LockInfo.java
+++ b/helix-lock/src/main/java/org/apache/helix/lock/LockInfo.java
@@ -34,7 +34,8 @@ public class LockInfo extends HelixProperty {
public static final long DEFAULT_TIMEOUT_LONG = -1L;
// default lock info represents the status of a unlocked lock
- public static final LockInfo defaultLockInfo = new LockInfo("");
+ public static final LockInfo defaultLockInfo =
+ new LockInfo(DEFAULT_OWNER_TEXT, DEFAULT_MESSAGE_TEXT, DEFAULT_TIMEOUT_LONG);
/**
* The keys to lock information
@@ -44,11 +45,11 @@ public class LockInfo extends HelixProperty {
}
/**
- * Initialize a LockInfo with a ZNRecord id, set all info fields to default data
+ * Initialize a default LockInfo instance
*/
- public LockInfo(String id) {
- super(id);
- resetLockInfo();
+ private LockInfo() {
+ super("LOCK");
+ setLockInfoFields(null, null, DEFAULT_TIMEOUT_LONG);
}
/**
@@ -56,8 +57,13 @@ public class LockInfo extends HelixProperty {
* @param znRecord The ZNRecord contains lock node data that used to initialize the LockInfo
*/
public LockInfo(ZNRecord znRecord) {
- super(znRecord);
- setNullLockInfoFieldsToDefault();
+ this();
+ if (znRecord != null) {
+ String ownerId = znRecord.getSimpleField(LockInfoAttribute.OWNER.name());
+ String message = znRecord.getSimpleField(LockInfoAttribute.MESSAGE.name());
+ long timeout = znRecord.getLongField(LockInfoAttribute.TIMEOUT.name(), DEFAULT_TIMEOUT_LONG);
+ setLockInfoFields(ownerId, message, timeout);
+ }
}
/**
@@ -67,45 +73,22 @@ public class LockInfo extends HelixProperty {
* @param timeout value of TIMEOUT attribute
*/
public LockInfo(String ownerId, String message, long timeout) {
- this(ownerId);
+ this();
setLockInfoFields(ownerId, message, timeout);
}
/**
- * Build a LOCKINFO instance that represents an unlocked lock states
- * @return the unlocked lock node LockInfo instance
- */
- public static LockInfo buildUnlockedLockInfo() {
- return new LockInfo("");
- }
-
- /**
* Set each field of lock info to user provided values if the values are not null, null values are set to default values
* @param ownerId value of OWNER attribute
* @param message value of MESSAGE attribute
* @param timeout value of TIMEOUT attribute
*/
- public void setLockInfoFields(String ownerId, String message, Long timeout) {
+ private void setLockInfoFields(String ownerId, String message, long timeout) {
_record.setSimpleField(LockInfoAttribute.OWNER.name(),
ownerId == null ? DEFAULT_OWNER_TEXT : ownerId);
_record.setSimpleField(LockInfoAttribute.MESSAGE.name(),
message == null ? DEFAULT_MESSAGE_TEXT : message);
- _record.setLongField(LockInfoAttribute.TIMEOUT.name(),
- timeout == null ? DEFAULT_TIMEOUT_LONG : timeout);
- }
-
- /**
- * Set all null values to default values in LockInfo, keep non-null values
- */
- private void setNullLockInfoFieldsToDefault() {
- setLockInfoFields(getOwner(), getMessage(), getTimeout());
- }
-
- /**
- * Reset the lock info to unlocked lock state
- */
- public void resetLockInfo() {
- setLockInfoFields(DEFAULT_OWNER_TEXT, DEFAULT_MESSAGE_TEXT, DEFAULT_TIMEOUT_LONG);
+ _record.setLongField(LockInfoAttribute.TIMEOUT.name(), timeout);
}
/**
@@ -135,46 +118,10 @@ public class LockInfo extends HelixProperty {
}
/**
- * Get the value for OWNER attribute of the lock from a ZNRecord
- * @return the owner id of the lock, empty string if there is no owner id set
- */
- public static String getOwner(ZNRecord znRecord) {
- if (znRecord == null) {
- return DEFAULT_OWNER_TEXT;
- }
- String owner = znRecord.getSimpleField(LockInfoAttribute.OWNER.name());
- return owner == null ? DEFAULT_OWNER_TEXT : owner;
- }
-
- /**
- * Get the value for MESSAGE attribute of the lock from a ZNRecord
- * @return the message of the lock, empty string if there is no message set
- */
- public static String getMessage(ZNRecord znRecord) {
- if (znRecord == null) {
- return DEFAULT_MESSAGE_TEXT;
- }
- String message = znRecord.getSimpleField(LockInfoAttribute.MESSAGE.name());
- return message == null ? DEFAULT_MESSAGE_TEXT : message;
- }
-
- /**
- * Get the value for TIMEOUT attribute of the lock from a ZNRecord
- * @return the expiring time of the lock, -1 if there is no timeout set
- */
- public static long getTimeout(ZNRecord znRecord) {
- if (znRecord == null) {
- return DEFAULT_TIMEOUT_LONG;
- }
- return znRecord.getLongField(LockInfoAttribute.TIMEOUT.name(), DEFAULT_TIMEOUT_LONG);
- }
-
- /**
- * Check if the lock has a owner id set
- * @return true if an owner id is set, false if not
+ * Check if a lock has timed out
+ * @return return true if the lock has timed out, otherwise return false.
*/
- public static boolean ownerIdSet(ZNRecord znRecord) {
- String ownerId = getOwner(znRecord);
- return !ownerId.equals(DEFAULT_OWNER_TEXT);
+ public boolean hasNotExpired() {
+ return System.currentTimeMillis() < getTimeout();
}
}
diff --git a/helix-lock/src/main/java/org/apache/helix/lock/LockScope.java b/helix-lock/src/main/java/org/apache/helix/lock/LockScope.java
index ff2d329..0fa5cfc 100644
--- a/helix-lock/src/main/java/org/apache/helix/lock/LockScope.java
+++ b/helix-lock/src/main/java/org/apache/helix/lock/LockScope.java
@@ -19,7 +19,14 @@
package org.apache.helix.lock;
+/**
+ * A predefined class to generate the lock path based on user input
+ */
public interface LockScope {
+ /**
+ * Get the lock path
+ * @return the path of the lock
+ */
String getPath();
}
diff --git a/helix-lock/src/main/java/org/apache/helix/lock/helix/HelixLockScope.java b/helix-lock/src/main/java/org/apache/helix/lock/helix/HelixLockScope.java
index 77907ea..8eec472 100644
--- a/helix-lock/src/main/java/org/apache/helix/lock/helix/HelixLockScope.java
+++ b/helix-lock/src/main/java/org/apache/helix/lock/helix/HelixLockScope.java
@@ -22,6 +22,7 @@ package org.apache.helix.lock.helix;
import java.util.List;
import org.apache.helix.lock.LockScope;
+import org.apache.helix.model.HelixConfigScope;
import org.apache.helix.util.StringTemplate;
@@ -35,13 +36,11 @@ public class HelixLockScope implements LockScope {
*/
public enum LockScopeProperty {
- CLUSTER(1),
+ CLUSTER(2),
PARTICIPANT(2),
- RESOURCE(3),
-
- PARTITION(4);
+ RESOURCE(2);
//the number of arguments required to generate a full path for the specific scope
final int _pathArgNum;
@@ -61,14 +60,6 @@ public class HelixLockScope implements LockScope {
public int getPathArgNum() {
return _pathArgNum;
}
-
- /**
- * Get the position of this argument from the input that used to generate the scope
- * @return the number of position of value for this property in the list of keys input
- */
- public int getArgPos() {
- return _pathArgNum - 1;
- }
}
/**
@@ -76,108 +67,34 @@ public class HelixLockScope implements LockScope {
*/
private static final StringTemplate template = new StringTemplate();
+ //TODO: Enrich the logic of path generation once we have a more detailed design
static {
- template.addEntry(LockScopeProperty.CLUSTER, 1, "/{clusterName}/LOCK");
+ template.addEntry(LockScopeProperty.CLUSTER, 2, "/{clusterName}/LOCK/CLUSTER/{clusterName}");
template.addEntry(HelixLockScope.LockScopeProperty.PARTICIPANT, 2,
- "/{clusterName}/LOCK/{participantName}");
- template.addEntry(HelixLockScope.LockScopeProperty.RESOURCE, 3,
- "/{clusterName}/LOCK/{participantName}/{resourceName}");
- template.addEntry(HelixLockScope.LockScopeProperty.PARTITION, 4,
- "/{clusterName}/LOCK/{participantName}/{resourceName}/{partitionName}");
+ "/{clusterName}/LOCK/PARTICIPANT/{participantName}");
+ template.addEntry(HelixLockScope.LockScopeProperty.RESOURCE, 2,
+ "/{clusterName}/LOCK/RESOURCE/{resourceName}");
}
- private final HelixLockScope.LockScopeProperty _type;
- private final String _clusterName;
- private final String _participantName;
- private final String _resourceName;
- private final String _partitionName;
-
- private final String _zkPath;
+ private final String _path;
/**
* Initialize with a type of scope and unique identifiers
* @param type the scope
- * @param zkPathKeys keys identifying a ZNode location
+ * @param pathKeys keys identifying a ZNode location
*/
- private HelixLockScope(HelixLockScope.LockScopeProperty type, List<String> zkPathKeys) {
+ public HelixLockScope(HelixLockScope.LockScopeProperty type, List<String> pathKeys) {
- if (zkPathKeys.size() != type.getPathArgNum()) {
+ if (pathKeys.size() != type.getPathArgNum()) {
throw new IllegalArgumentException(
type + " requires " + type.getPathArgNum() + " arguments to get znode, but was: "
- + zkPathKeys);
- }
-
- _type = type;
-
- //Initialize the name fields for various scope
- _clusterName = zkPathKeys.get(LockScopeProperty.CLUSTER.getArgPos());
-
- if (type.getPathArgNum() >= LockScopeProperty.PARTICIPANT.getPathArgNum()) {
- _participantName = zkPathKeys.get(LockScopeProperty.PARTICIPANT.getArgPos());
- } else {
- _participantName = null;
- }
-
- if (type.getPathArgNum() >= LockScopeProperty.RESOURCE.getPathArgNum()) {
- _resourceName = zkPathKeys.get(LockScopeProperty.RESOURCE.getArgPos());
- } else {
- _resourceName = null;
- }
-
- if (type.getPathArgNum() >= LockScopeProperty.PARTITION.getPathArgNum()) {
- _partitionName = zkPathKeys.get(LockScopeProperty.PARTITION.getArgPos());
- } else {
- _partitionName = null;
+ + pathKeys);
}
-
- _zkPath = template.instantiate(type, zkPathKeys.toArray(new String[0])).toUpperCase();
- }
-
- /**
- * Get the scope
- * @return the type of scope
- */
- public HelixLockScope.LockScopeProperty getType() {
- return _type;
- }
-
- /**
- * Get the cluster name if it exists
- * @return the cluster name
- */
- public String getClusterName() {
- return _clusterName;
- }
-
- /**
- * Get the participant name if it exists
- * @return the participant name
- */
- public String getParticipantName() {
- return _participantName;
- }
-
- /**
- * Get the resource name if it exists
- * @return the resource name
- */
- public String getResourceName() {
- return _resourceName;
- }
-
- /**
- * Get the partition name if it exists
- * @return the partition name
- */
- public String getPartitionName() {
- return _partitionName;
+ _path = template.instantiate(type, pathKeys.toArray(new String[0]));
}
@Override
- /**
- * Get the path to the corresponding ZNode
- * @return a Zookeeper path
- */ public String getPath() {
- return _zkPath;
+ public String getPath() {
+ return _path;
}
}
diff --git a/helix-lock/src/main/java/org/apache/helix/lock/helix/ZKHelixNonblockingLock.java b/helix-lock/src/main/java/org/apache/helix/lock/helix/ZKHelixNonblockingLock.java
index 478be95..41de4a7 100644
--- a/helix-lock/src/main/java/org/apache/helix/lock/helix/ZKHelixNonblockingLock.java
+++ b/helix-lock/src/main/java/org/apache/helix/lock/helix/ZKHelixNonblockingLock.java
@@ -70,6 +70,9 @@ public class ZKHelixNonblockingLock implements HelixLock {
private ZKHelixNonblockingLock(String lockPath, String zkAddress, Long timeout, String lockMsg,
String userId) {
_lockPath = lockPath;
+ if (timeout < 0) {
+ throw new IllegalArgumentException("The expiration time cannot be negative.");
+ }
_timeout = timeout;
_lockMsg = lockMsg;
_userId = userId;
@@ -87,10 +90,7 @@ public class ZKHelixNonblockingLock implements HelixLock {
} else {
deadline = System.currentTimeMillis() + _timeout;
}
- LockInfo lockInfo = new LockInfo(_userId);
- lockInfo.setLockInfoFields(_userId, _lockMsg, deadline);
-
- LockUpdater updater = new LockUpdater(lockInfo);
+ LockUpdater updater = new LockUpdater(new LockInfo(_userId, _lockMsg, deadline));
return _baseDataAccessor.update(_lockPath, updater, AccessOption.PERSISTENT);
}
@@ -102,57 +102,15 @@ public class ZKHelixNonblockingLock implements HelixLock {
}
@Override
- public LockInfo getLockInfo() {
+ public LockInfo getCurrentLockInfo() {
ZNRecord curLockInfo = _baseDataAccessor.get(_lockPath, null, AccessOption.PERSISTENT);
return new LockInfo(curLockInfo);
}
@Override
- public boolean isOwner() {
- LockInfo lockInfo = getLockInfo();
- return userIdMatches(lockInfo) && !hasTimedOut(lockInfo);
- }
-
- /**
- * Check if a lock has timed out
- * @return return true if the lock has timed out, otherwise return false.
- */
- private boolean hasTimedOut(LockInfo lockInfo) {
- return System.currentTimeMillis() >= lockInfo.getTimeout();
- }
-
- /**
- * Check if a lock has timed out with lock information stored in a ZNRecord
- * @return return true if the lock has timed out, otherwise return false.
- */
- private boolean hasTimedOut(ZNRecord znRecord) {
- return System.currentTimeMillis() >= LockInfo.getTimeout(znRecord);
- }
-
- /**
- * Check if the current user Id matches with the owner Id in a lock info
- * @return return true if the two ids match, otherwise return false.
- */
- private boolean userIdMatches(LockInfo lockInfo) {
- return lockInfo.getOwner().equals(_userId);
- }
-
- /**
- * Check if a user id in the ZNRecord matches current user's id
- * @param znRecord ZNRecord contains lock information
- * @return true if the ids match, false if not or ZNRecord does not contain user id information
- */
- private boolean userIdMatches(ZNRecord znRecord) {
- return LockInfo.getOwner(znRecord).equals(_userId);
- }
-
- /**
- * Check if the lock node has a current owner
- * @param znRecord Lock information in format of ZNRecord
- * @return true if the lock has a current owner that the ownership has not be timed out, otherwise false
- */
- private boolean hasNonExpiredOwner(ZNRecord znRecord) {
- return LockInfo.ownerIdSet(znRecord) && !hasTimedOut(znRecord);
+ public boolean isCurrentOwner() {
+ LockInfo lockInfo = getCurrentLockInfo();
+ return lockInfo.getOwner().equals(_userId) && lockInfo.hasNotExpired();
}
/**
@@ -172,8 +130,9 @@ public class ZKHelixNonblockingLock implements HelixLock {
@Override
public ZNRecord update(ZNRecord current) {
// If no one owns the lock, allow the update
- // If the lock owner id matches user id, allow the update
- if (!hasNonExpiredOwner(current) || userIdMatches(current)) {
+ // If the user is the current lock owner, allow the update
+ LockInfo curLockInfo = new LockInfo(current);
+ if (!curLockInfo.hasNotExpired() || 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
diff --git a/helix-lock/src/test/java/org/apache/helix/lock/helix/TestZKHelixNonblockingLock.java b/helix-lock/src/test/java/org/apache/helix/lock/helix/TestZKHelixNonblockingLock.java
index e79922d..39205af 100644
--- a/helix-lock/src/test/java/org/apache/helix/lock/helix/TestZKHelixNonblockingLock.java
+++ b/helix-lock/src/test/java/org/apache/helix/lock/helix/TestZKHelixNonblockingLock.java
@@ -57,14 +57,12 @@ public class TestZKHelixNonblockingLock extends ZkTestBase {
List<String> pathKeys = new ArrayList<>();
pathKeys.add(_clusterName);
- pathKeys.add("participant_name");
- pathKeys.add("resource_name");
- pathKeys.add("partition_name");
+ pathKeys.add(_clusterName);
- _participantScope = new HelixLockScope(HelixLockScope.LockScopeProperty.PARTITION, pathKeys);
+ _participantScope = new HelixLockScope(HelixLockScope.LockScopeProperty.CLUSTER, pathKeys);
_lockPath = _participantScope.getPath();
- _lock = new ZKHelixNonblockingLock(_participantScope, ZK_ADDR, Long.MAX_VALUE,
- _lockMessage, _userId);
+ _lock = new ZKHelixNonblockingLock(_participantScope, ZK_ADDR, Long.MAX_VALUE, _lockMessage,
+ _userId);
}
@BeforeMethod
@@ -81,17 +79,16 @@ public class TestZKHelixNonblockingLock extends ZkTestBase {
Assert.assertTrue(_gZkClient.exists(_lockPath));
// Get lock information
- LockInfo lockInfo = _lock.getLockInfo();
+ LockInfo lockInfo = _lock.getCurrentLockInfo();
Assert.assertEquals(lockInfo.getOwner(), _userId);
- Assert.assertEquals(lockInfo.getMessage(),
- _lockMessage);
+ Assert.assertEquals(lockInfo.getMessage(), _lockMessage);
// Check if the user is lock owner
- Assert.assertTrue(_lock.isOwner());
+ Assert.assertTrue(_lock.isCurrentOwner());
// Release lock
_lock.releaseLock();
- Assert.assertFalse(_lock.isOwner());
+ Assert.assertFalse(_lock.isCurrentOwner());
}
@Test
@@ -101,16 +98,16 @@ public class TestZKHelixNonblockingLock extends ZkTestBase {
String fakeUserID = UUID.randomUUID().toString();
ZNRecord fakeRecord = new ZNRecord(fakeUserID);
fakeRecord.setSimpleField(LockInfo.LockInfoAttribute.OWNER.name(), fakeUserID);
- fakeRecord.setSimpleField(LockInfo.LockInfoAttribute.TIMEOUT.name(),
- String.valueOf(Long.MAX_VALUE));
+ fakeRecord
+ .setSimpleField(LockInfo.LockInfoAttribute.TIMEOUT.name(), String.valueOf(Long.MAX_VALUE));
_gZkClient.create(_lockPath, fakeRecord, CreateMode.PERSISTENT);
// Check if the user is lock owner
- Assert.assertFalse(_lock.isOwner());
+ Assert.assertFalse(_lock.isCurrentOwner());
// Acquire lock
Assert.assertFalse(_lock.acquireLock());
- Assert.assertFalse(_lock.isOwner());
+ Assert.assertFalse(_lock.isCurrentOwner());
// Release lock
Assert.assertFalse(_lock.releaseLock());
@@ -129,11 +126,11 @@ public class TestZKHelixNonblockingLock extends ZkTestBase {
// Acquire lock
Assert.assertTrue(_lock.acquireLock());
- Assert.assertTrue(_lock.isOwner());
+ Assert.assertTrue(_lock.isCurrentOwner());
// Release lock
Assert.assertTrue(_lock.releaseLock());
- Assert.assertFalse(_lock.isOwner());
+ Assert.assertFalse(_lock.isCurrentOwner());
}
@Test
@@ -141,8 +138,8 @@ public class TestZKHelixNonblockingLock extends ZkTestBase {
List<Callable<Boolean>> threads = new ArrayList<>();
for (int i = 0; i < 2; i++) {
ZKHelixNonblockingLock lock =
- new ZKHelixNonblockingLock(_participantScope, ZK_ADDR, Long.MAX_VALUE,
- _lockMessage, UUID.randomUUID().toString());
+ new ZKHelixNonblockingLock(_participantScope, ZK_ADDR, Long.MAX_VALUE, _lockMessage,
+ UUID.randomUUID().toString());
threads.add(new TestSimultaneousAcquireLock(lock));
}
Map<String, Boolean> resultMap = TestHelper.startThreadsConcurrently(threads, 1000);