Fix: ShouldSkipBlockingWait should still acquire a dead lock if tried for longer than TTL #99
Conversation
moshegood
changed the title
| if (notMyLocks.containsKey(id) && | ||
| notMyLocks.get(id).getRecordVersionNumber() | ||
| .equals(existingLock.get().getRecordVersionNumber())) { | ||
| itReallyIsExpired = oldCopy.isExpired(); |
There was a problem hiding this comment.
I think this approach works, with the following downsides:
- The very first acquire call on an existing lock will throw, always. There should be a separate check for
notMyLocks.containsKey(id). - Ownership changes for expired locks will take twice as long since
oldCopy.isExpired()will return true only afterlease_durationsecs. Then the rest of the acquire will take anotherlease_durationseconds.
There was a problem hiding this comment.
For item 2, maybe try calling upsertAndMonitorExpiredLock() immediately when it expired?
There was a problem hiding this comment.
Item 2 fixed in latest push.
Item 1: the first call on an existing lock
- there is no way to know how long it has been acquired
- the only possible options is to return that the lock is held
There was a problem hiding this comment.
There is another option which is to block on the first call. If it returned false, then the following calls should fail fast.
There was a problem hiding this comment.
If the user wants to block to wait for a lease, they should not set: ShouldSkipBlockingWait
There was a problem hiding this comment.
I am fine with that approach as long as it is made clear (in the README?) that a return value of false does not only mean the lease is held. It may also mean that the client is, with the information that it has, not able to determine whether the lease is owned or not.
There was a problem hiding this comment.
Probably outside the scope of this PR, but instead of using the skipBlockingWait property there can be two acquire methods - one for blocking and another for non-blocking.
There was a problem hiding this comment.
See #102 for updates to the README.
There was a problem hiding this comment.
The message here needs to be updated as well since all acquires during the first LEASE_DURATION seconds will fail for an existing, expired lock.
src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java
Show resolved
Hide resolved
| @@ -233,6 +233,7 @@ public class AmazonDynamoDBLockClient implements Runnable, Closeable { | |||
| private final boolean holdLockOnServiceUnavailable; | |||
| private final String ownerName; | |||
| private final ConcurrentHashMap<String, LockItem> locks; | |||
| private final ConcurrentHashMap<String, LockItem> notMyLocks; | |||
There was a problem hiding this comment.
Do we need to purge entries from the map eventually?
There was a problem hiding this comment.
If an app takes out leases with constantly changing names, then this can grow unboundedly.
If the lease names that it attempts to acquire are limited, then not.
moshegood
force-pushed
the
moshe/ShouldSkipBlockingWait/eventually.should.work
branch
from
949cb9b to
d592483
Compare
moshegood
force-pushed
the
moshe/ShouldSkipBlockingWait/eventually.should.work
branch
from
d592483 to
dbc76cc
Compare
moshegood
force-pushed
the
moshe/ShouldSkipBlockingWait/eventually.should.work
branch
from
5fde650 to
f620e0a
Compare
Issue 44
Description of changes:
When a lock is not acquired because the
ShouldSkipBlockingWaithas been set, we cache the data pulled from DynamoDB. In future calls, if the recordVersion matches the cached version, we check is the cached version to see if it is expired rather than the freshly pulled copy, as the freshly pulled copy will never be expired.