[fix][client] Fix waiting lookup requests not dispatched on timeout in ClientCnx#26143
Open
geniusjoe wants to merge 1 commit into
Open
[fix][client] Fix waiting lookup requests not dispatched on timeout in ClientCnx#26143geniusjoe wants to merge 1 commit into
geniusjoe wants to merge 1 commit into
Conversation
Contributor
Author
|
@congbobo184 |
This file contains hidden or 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
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.
main pr #25038
Motivation
PR #25038 fixed the semaphore leak issue by adding a
TimeoutExceptioncheck in thewhenCompletecallback ofnewLookup(). However, it did not fully address the waiting queue starvation problem.When a lookup request times out in
checkRequestTimeout(), the code only callspendingRequests.remove()and relies on thewhenCompletecallback to release the semaphore. While the semaphore is correctly released,getAndRemovePendingLookupRequest()— which is responsible for polling and dispatching the next waiting request fromwaitingLookupRequests— is never invoked on the timeout path.This means: if all concurrent lookup slots time out simultaneously (e.g., during a broker GC pause or network partition), the waiting requests remain stuck indefinitely even though semaphore permits become available. They will never be dispatched until a new non-timeout lookup response arrives.
Modifications
Optimized the timeout handling path in
ClientCnx.java:checkRequestTimeout(): ForLookup-type requests, usegetAndRemovePendingLookupRequest()instead of plainpendingRequests.remove(). This ensures the next waiting request inwaitingLookupRequestsis polled and dispatched upon timeout, rather than just releasing the semaphore.newLookup()whenCompletecallback: Removed theTimeoutExceptioncheck sincegetAndRemovePendingLookupRequest()already handles semaphore release and queue driving. Keeping it would cause a double-release.Verifying this change
This change added tests and can be verified as follows:
testLookupTimeoutReleasesSemaphore— verifies that the semaphore is properly released after a lookup timeout.testLookupTimeoutDrivesWaitingQueue— verifies that when a concurrent lookup times out, the next waiting request in the queue is dispatched.testMultipleLookupTimeoutsNoSemaphoreLeak— verifies that with multiple concurrent and queued lookups all timing out, every request eventually completes and no semaphore permits are leaked.Does this pull request potentially affect one of the following parts:
Local workflow:
geniusjoe#1