slowsoul commented on code in PR #2698:
URL: https://github.com/apache/helix/pull/2698#discussion_r1395007036
##########
helix-lock/src/main/java/org/apache/helix/lock/helix/ZKDistributedNonblockingLock.java:
##########
@@ -312,16 +312,20 @@ public ZNRecord update(ZNRecord current) {
return _record;
}
- // higher priority lock request will try to preempt current lock owner
- if (!isCurrentOwner(curLockInfo) && _priority >
curLockInfo.getPriority()) {
+ LockInfo unlockOrLockRequestLockInfo = new LockInfo(_record);
+ int unlockOrLockRequestPriority =
unlockOrLockRequestLockInfo.getPriority();
+ // higher priority lock request will try to preempt current lock owner.
And any unlock request
+ // from non-lock owners will always be blocked because the default
priority of unlock request
+ // is 0. And it would always be less than or equal to the priority value
of the current lock.
+ if (!isCurrentOwner(curLockInfo) && unlockOrLockRequestPriority >
curLockInfo.getPriority()) {
Review Comment:
This should work properly, but one thing is that unlock request will get the
HelixException in the end of this method with text "failed to acquire lock",
which can be confusing. I think it might be better to provide different
exception message for each case or a general exception message that covers both
cases.
##########
helix-lock/src/test/java/org/apache/helix/lock/helix/TestZKHelixNonblockingLock.java:
##########
@@ -141,6 +141,23 @@ public void testAcquireLockWhenExistingLockExpired() {
Assert.assertFalse(_lock.isCurrentOwner());
}
+ @Test
+ public void testOtherClientsAcquireLockFromExistingLockExpired() {
+ // Fake condition when the lock owner is not current user
+ 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.setIntField(LockInfo.LockInfoAttribute.PRIORITY.name(), 0);
Review Comment:
I think it's better to also have a test where there is no PRIORITY field set
in the ZNRecord, this way the test can verify the case where the lock ZNRecord
is written by the old helix-lock lib.
I would also suggest verifying `_lock.tryLock()` returns false instead of
throwing exception when there's no PRIORITY field set in the ZNRecord or
PRIORITY field is explicitly set to 0 in the ZNRecord, as this was the main
compatibility issue that's preventing migration from old to new helix-lock.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
