Added support for faster shard reallocation on graceful shutdown #1005
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mathieufortin01
commented
Oct 18, 2022
| * Interval at which the lease taker will execute. | ||
| * If unspecified, an interval will be calculated based on the lease duration. | ||
| */ | ||
| private long leaseTakerIntervalMillis = -1L; |
There was a problem hiding this comment.
Exposing this property to clients.
| * If leases should be evicted or not on shutdown requested. | ||
| * By default, leases are not evicted. | ||
| */ | ||
| private boolean evictLeaseOnShutdown = false; |
There was a problem hiding this comment.
New property. If false, same behavior as today which is wait for lease to be expired before taking it.
| @@ -191,9 +191,9 @@ synchronized Map<String, Lease> takeLeases(Callable<Long> timeProvider) | |||
| return takenLeases; | |||
| } | |||
|
|
|||
| List<Lease> expiredLeases = getExpiredLeases(); | |||
| List<Lease> availableLeases = getAvailableLeases(); | |||
There was a problem hiding this comment.
Changes in this file are all related to changing expired lease concept for available lease concept.
| @@ -482,7 +491,7 @@ private Set<Lease> computeLeasesToTake(List<Lease> expiredLeases) { | |||
| } | |||
|
|
|||
| } finally { | |||
| scope.addData("ExpiredLeases", expiredLeases.size(), StandardUnit.COUNT, MetricsLevel.SUMMARY); | |||
| scope.addData("AvailableLeases", availableLeases.size(), StandardUnit.COUNT, MetricsLevel.SUMMARY); | |||
There was a problem hiding this comment.
Not sure whats the best way to go here.
| @@ -48,21 +45,19 @@ | |||
| import software.amazon.kinesis.retrieval.kpl.ExtendedSequenceNumber; | |||
|
|
|||
| @RunWith(MockitoJUnitRunner.class) | |||
| public class DynamoDBLeaseCoordinatorIntegrationTest { | |||
| private static final int ATTEMPTS = 20; | |||
| public class DynamoDBLeaseCoordinatorIntegrationTest extends LeaseIntegrationTest { | |||
There was a problem hiding this comment.
Now extends from LeaseIntegrationTest, so I could remove a whole bunch of test setup.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available: #845
Description of changes:
We have this latency issue in production where on graceful shutdown we have seen up to 40s before records resume processing on a new container.
The cause of it is that on graceful shutdown, the lease stays owned. Others wait for lease to expire before picking them up. See #845 for details, this is pretty well explained.
The changes im proposing here are:
With these changes, I was able to go below 1s before records resumed consumption.
So I would like some opinions on this. We will test this in various envs and see the impacts. If this goes through this would be a contribution by Autodesk.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.