Treat unassigned leases (as well as expired ones) as available-to-be-taken #848
Conversation
rtyley
force-pushed
the
treat-unassigned-leases-as-available-to-be-taken
branch
2 times, most recently
from
613a96f
to
d0a2b95
Compare
rtyley
force-pushed
the
treat-unassigned-leases-as-available-to-be-taken
branch
from
d0a2b95
to
db34266
Compare
rtyley
force-pushed
the
treat-unassigned-leases-as-available-to-be-taken
branch
4 times, most recently
from
99822ea
to
5cdc9b5
Compare
|
I've rebased this PR on top of the latest The tests that @shanmsac introduced with #861 still pass with this updated PR, though I have tweaked one assertion to cope with the fact that the updated I hope this PR is still worth looking at, though obviously, having already managed to break things once with #847, I can understand why you might not trust this subsequent PR! |
| .filter(lease -> !lease.isUnassigned() && !availableLeasesSet.contains(lease)) | ||
| .collect(groupingBy(Lease::leaseOwner, summingInt(lease -> 1))); |
There was a problem hiding this comment.
Here filtering for !lease.isUnassigned() is what now stops the groupingBy Lease::leaseOwner from failing with the NullPointerException: element cannot be mapped to a null key error reported in #861 - given that unassigned leases are filtered out, there is no attempt to insert a null key entry in the map, so the code doesn't crash.
There was a problem hiding this comment.
Do we still want to keep this change given that #861 has been merged?
…taken If a lease is 'unassigned' (it has no lease owner) then it should be considered available for taking in `DynamoDBLeaseTaker`. Prior to this change, the only ways `DynamoDBLeaseTaker` could take leases for a scheduler was either by incremental lease stealing, or waiting for the lease to expire by not having been updated in `failoverTimeMillis` - which could be slow if `failoverTimeMillis` was set reasonably high (with it set to just 30s, I've seen new instances take over 3 minutes to take all leases from old instances in a deployment). This would be one half of the a fix for awslabs#845 - the other half of the fix is invoking `evictLease()` (setting the lease owner to null) on graceful shutdown of a scheduler.
rtyley
force-pushed
the
treat-unassigned-leases-as-available-to-be-taken
branch
from
5cdc9b5
to
f9d5077
Compare
| .filter(lease -> !lease.isUnassigned() && !availableLeasesSet.contains(lease)) | ||
| .collect(groupingBy(Lease::leaseOwner, summingInt(lease -> 1))); |
There was a problem hiding this comment.
Do we still want to keep this change given that #861 has been merged?
If a lease is 'unassigned' (it has no lease owner) then it should be considered available for taking in
DynamoDBLeaseTaker. Prior to this change, the only waysDynamoDBLeaseTakercould take leases for a scheduler was either by incremental lease stealing, or waiting for the lease to expire by not having been updated infailoverTimeMillis- which could be slow iffailoverTimeMilliswas set reasonably high (with it set to just 30s, I've seen new instances take over 3 minutes to take all leases from old instances in a deployment).This would be one half of a fix for #845 - the other half is invoking
evictLease()(setting the lease owner to null) on graceful shutdown of a scheduler.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.