Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get unassigned leases in leasesToTake #1320

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

brendan-p-lynch
Copy link
Contributor

Currently when considering leases to take we only consider leases that are expired. This creates a delay for leases that have no owner unnecessarily. We should instead consider all unassigned leases as a potential lease to take

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

* @param asOfNanos time in nanoseconds to check expiration as-of
* @return true if lease lease is ready te taken
*/
public boolean isAvailable(long leaseDurationNanos, long asOfNanos) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer a Duration type instead of long nanos.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we have this all the way up I'm not going to change it right now.

Comment on lines 376 to 384
List<Lease> availableLeases = new ArrayList<>();

for (Lease lease : allLeases.values()) {
if (lease.isExpired(leaseDurationNanos, lastScanTimeNanos)) {
expiredLeases.add(lease);
if (lease.isAvailable(leaseDurationNanos, lastScanTimeNanos)) {
availableLeases.add(lease);
}
}

return expiredLeases;
return availableLeases;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return allLeases.values().stream()
    .filter(lease->lease.isAvailable(...))
    .collect(toList())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in next commit

/**
* @param leaseDurationNanos duration of lease in nanoseconds
* @param asOfNanos time in nanoseconds to check expiration as-of
* @return true if lease lease is ready te taken
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in next commit

@brendan-p-lynch brendan-p-lynch merged commit 34fe58c into awslabs:master Apr 30, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants