Fix request processing scheduling #127464
Conversation
idegtiarenko
added
>non-issue
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| @@ -265,9 +265,8 @@ void onAfter(DriverCompletionInfo info) { | |||
| concurrentRequests.release(); | |||
| } | |||
|
|
|||
| if (pendingRetries.isEmpty() == false && remainingUnavailableShardResolutionAttempts.decrementAndGet() >= 0) { | |||
| if (sendingLock.isHeldByCurrentThread()) { | |||
There was a problem hiding this comment.
I think isHeldByCurrentThread should be used for assertions or debugging purposes, not in production code.
There was a problem hiding this comment.
Agree. let me find a way to replace it.
| pendingRetries.add(shardId); | ||
| if (pendingRetries == null && remainingUnavailableShardResolutionAttempts.decrementAndGet() >= 0) { | ||
| pendingRetries = new HashSet<>(); | ||
| sendingLock.lock(); |
There was a problem hiding this comment.
I am concerned that we are scattering sendingLock#lock and sendingLock#unlock in two different places. Can we keep them close?
There was a problem hiding this comment.
They are in the same inner listener, guarding the same structure at the moment.
They were previously in the same method before but that was not enough and caused a bug.
I suspect we could do this by creating a releasable inner wrapper class on top of pendingRetries = new HashSet<>() but that sounds like an overkill not really helping with readability.
There was a problem hiding this comment.
@idegtiarenko Sorry, I should have provided more detail. My concern is that we acquire the sending lock in maybeScheduleRetry and release it in onAfter, which is linked to the status of pendingRetries. While the implementation is technically correct, I think we should stick to the simplest lock pattern unless there is a strong reason to do otherwise:
lock/tryLock
try {
...
} finally {
unlock
}
I think we can follow this lock pattern in the DataNodeRequestSender class.
There was a problem hiding this comment.
This sounds a lot like the original implementation (within onAfter) that was not correct.
Do you see a way how to implement this suggestion while still locking conditionally (only if shard movement is detected) and blocking concurrent requests while it is detected that new shard location resolution is required?
There was a problem hiding this comment.
As discussed yesterday, I moved retry scheduling to trySendingRequestsForPendingShards in 8a0dcc6
This change fix concurrency around handling moved shards.
The test was failing as the shard failures were visible before retry was processed. In order to fix it the error handling is updated to:
Closes: #127168