Repository navigation
Fix stuck leases when worker restarts with pending checkpoint leases - #1718
Conversation
GitHub Actions
Failure
fbb7ab0 to
3c8b7b0
Compare
GitHub Actions
Inactive
| if (!isCheckpointOwner(lease)) { | ||
| log.warn( | ||
| "Worker {} not renewing shutdown-requested lease {} with checkpointOwner {}", | ||
| workerIdentifier, | ||
| lease, | ||
| lease.checkpointOwner()); | ||
| ownedLeases.remove(leaseKey); |
There was a problem hiding this comment.
Is this part necessary or extra safety precaution? I don't see how the pendingCheckpoint lease can be added to the ownedLeases anymore after the checkpointOwner check in initialize() line 499 and the existing check for leaseDiscoverer https://github.com/awslabs/amazon-kinesis-client/blob/master/amazon-kinesis-client/src/main/java/software/amazon/kinesis/leases/dynamodb/DynamoDBLeaseDiscoverer.java#L121
There was a problem hiding this comment.
This is for extra safety precaution to prevent any future method to accidentally renew leases that they shouldn't renew
|
|
||
| private void assignLease(final Lease lease, final WorkerMetricStats workerMetrics) { | ||
| if (nonNull(lease.actualOwner()) && lease.actualOwner().equals(workerMetrics.getWorkerId())) { | ||
| private void assignLease(final Lease lease, final WorkerMetricStats workerMetrics, boolean isExpiredLease) { |
| return renewLease(lease, false); | ||
| } | ||
|
|
||
| private boolean isCheckpointOwner(Lease lease) { |
| final WorkerMetricStats workerToAssignLease = assignableWorkerSortedByAvailableCapacity.poll(); | ||
| if (nonNull(workerToAssignLease)) { | ||
| assignLease(lease, workerToAssignLease); | ||
| assignLease(lease, workerToAssignLease, true); |
There was a problem hiding this comment.
Technically can be unassigned but not expired right? Is it possible to be unassigned but still have pendingCheckpoint?
There was a problem hiding this comment.
Correct. In KCL, we don't expect leaseOwner attribute to be removed after lease is added to the system and assigned once. So unassigned leases should never have pending checkpoint unless they get manually edited for example
Description of changes:
During graceful lease handoff from Worker A to Worker B, a lease can become stuck in a pending checkpoint state when Worker A fails to complete the checkpoint (e.g., due to spurious retries). This leaves the lease with checkpointOwner set to Worker A and leaseOwner set to Worker B.(A valid state)
If Worker B is running, it will not try to acquire the lease because it sees the lease is in pending checkpoint state and leaseDiscover would prevent it to acquire the lease. However, if Worker B restarts and initializes its
LeaseCoordinator, it discovers this lease during the initialize() call. The worker incorrectly acquires and immediately attempts to shut down the lease. The shutdown logic fails to release the lease because the worker never explicitly drops it, causing the heartbeat to continue indefinitely and leaving the lease stuck.
The change adds logic in the initialize method to also not acquire pendingCheckpoint leases as done in leaseDiscoverer. It also adds logic to not renew leases that's not in an expected state
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.