shuturmurgh opened a new issue, #19611:
URL: https://github.com/apache/pinot/issues/19611
## Summary
`BaseTableDataManager` acquires the per-segment lock and the server-wide
`SegmentReloadSemaphore` in **opposite orders** on the refresh and reload paths:
| Path | Order | Location on `master` (`7f79c65`) |
|---|---|---|
| `replaceSegmentInternal` → `doReplaceSegment` (refresh) | segment lock →
semaphore | `BaseTableDataManager.java:537` then `:556` |
| `reloadSegment` (reload) | semaphore → segment lock |
`BaseTableDataManager.java:1106` then `:1108` |
When a refresh and a reload target the same segment, this deadlocks
permanently. Because `max.parallel.refresh.threads` defaults to **1**
(`HelixInstanceDataManagerConfig:259`), the single permit is then held forever
and *every* segment refresh and reload on the server stops — for all tables,
not just the two involved. Neither acquisition has a timeout
(`SegmentReloadSemaphore.acquire` calls bare `Semaphore.acquire()`; the reload
path calls bare `Lock.lock()`), so the server never recovers without a restart.
We hit this in production and it silently stopped all segment refresh on
four servers for ~62 hours.
## Setup
An offline job refreshes a fixed set of segments for several tables daily.
Each run rewrites the segments under the same names with new CRCs, so every run
triggers `SegmentRefreshMessage` for each segment.
## Deadlock timeline
1. `table_B`'s segments are uploaded with modified CRCs, triggering
refreshes. `table_B` is missing its Lucene text index in the generated
segments, so `SegmentPreProcessor` builds it at load time — **~20 minutes per
segment**, all of it while holding the server-wide semaphore. A backlog forms.
2. `table_A`'s segments are uploaded with modified CRCs, triggering
refreshes for `table_A_00001` … `table_A_00010`. The refresh for
`table_A_00001` takes that segment's lock, then blocks waiting for the
semaphore, **still holding the segment lock**.
3. A table reload is issued for `table_A`. The reload for `table_A_00001`
acquires the semaphore first, then reaches for the segment lock.
4. Deadlock. The refresh holds the segment lock and waits for the permit;
the reload holds the permit and waits for the segment lock. With one permit,
all other queued refreshes now wait forever too, and since each occupies one of
the 40 threads in the Helix message-handler pool
(`HelixTaskExecutor.DEFAULT_PARALLEL_TASKS`), no refresh or reload for any
table can start.
## Evidence
### Thread dump
Refresh handler — **holds** the segment lock, waiting on the semaphore:
```
"HelixTaskExecutor-message_handle_thread_62" daemon prio=5 waiting on
condition
java.lang.Thread.State: WAITING (parking)
at jdk.internal.misc.Unsafe.park([email protected]/Native Method)
- parking to wait for <0x0000ffbeef659ca0> (a
java.util.concurrent.Semaphore$FairSync)
at
java.util.concurrent.Semaphore.acquire([email protected]/Semaphore.java:318)
at
org.apache.pinot.segment.local.utils.SegmentReloadSemaphore.acquire(SegmentReloadSemaphore.java:39)
at
org.apache.pinot.core.data.manager.BaseTableDataManager.doReplaceSegment(BaseTableDataManager.java:481)
at
org.apache.pinot.core.data.manager.BaseTableDataManager.replaceSegment(BaseTableDataManager.java:464)
at
org.apache.pinot.server.starter.helix.HelixInstanceDataManager.replaceSegment(HelixInstanceDataManager.java:343)
at
org.apache.pinot.server.starter.helix.SegmentMessageHandlerFactory$SegmentRefreshMessageHandler.handleMessage(SegmentMessageHandlerFactory.java:110)
at org.apache.helix.messaging.handling.HelixTask.call(HelixTask.java:97)
Locked ownable synchronizers:
- <0x0000ffbe9cbfea00> (a
java.util.concurrent.locks.ReentrantLock$NonfairSync)
```
Reload worker — **holds** the permit, waiting on that same lock
`0x0000ffbe9cbfea00`:
```
"segment-reload-thread-0" prio=5 waiting on condition
java.lang.Thread.State: WAITING (parking)
at jdk.internal.misc.Unsafe.park([email protected]/Native Method)
- parking to wait for <0x0000ffbe9cbfea00> (a
java.util.concurrent.locks.ReentrantLock$NonfairSync)
at
java.util.concurrent.locks.ReentrantLock.lock([email protected]/ReentrantLock.java:322)
at
org.apache.pinot.core.data.manager.BaseTableDataManager.reloadSegment(BaseTableDataManager.java:803)
at
org.apache.pinot.core.data.manager.BaseTableDataManager.reloadSegment(BaseTableDataManager.java:790)
at
org.apache.pinot.core.data.manager.BaseTableDataManager.lambda$reloadSegments$2(BaseTableDataManager.java:747)
at
java.util.concurrent.CompletableFuture$AsyncRun.run([email protected]/CompletableFuture.java:1804)
```
(Line numbers above are from the version we ran; on `master` they map to
`doReplaceSegment:556`, `replaceSegmentInternal:537` and `reloadSegment:1108`.)
Pool census — 40 idle state-transition threads, and 40 user-defined-message
threads all blocked:
| Count | State |
|---|---|
| 37 | refresh, blocked at `doReplaceSegment:481` on the semaphore, each
holding one segment lock |
| 2 | `reloadAllSegments:569` → `reloadSegments:756`, blocked on
`CompletableFuture.get` |
| 1 | refresh, blocked at `replaceSegment:462` on the same segment lock |
Two dumps taken 21 minutes apart show **byte-identical CPU counters** for
every participant (thread 62: `2455564.32 ms` in both) and an identical set of
37 waiters — a hang, not slow progress.
### Semaphore queue growth
```
10:45:12 table_B_00007 Acquired lock to reload segment (lock-time=0ms,
queue-length=0)
10:46:46 table_B_00009 Waiting for lock to reload, queue-length: 1
11:04:47 table_B_00001 Waiting for lock to reload, queue-length: 8
11:05:53 table_B_00002 Acquired lock to reload segment
(lock-time=1205789ms, queue-length=8)
11:23:32 table_A_00001 Waiting for lock to reload, queue-length: 8 <-
holds its segment lock
11:26:25 table_B_00003 Acquired lock to reload segment
(lock-time=2378918ms, queue-length=8)
```
## Note: `jstack` does not report this as a deadlock
`jstack -l` prints no `Found one Java-level deadlock` section. The JVM's
detector only follows monitors and exclusively-owned AQS synchronizers; a
`Semaphore` has no owning thread, so a cycle passing through one is invisible
to it. This makes the bug considerably harder to attribute — the observable
symptom is just "Helix completed-task counter flat, 40 active threads, restart
fixes it," which is easy to misdiagnose.
## Proposed fix
Make `reloadSegment` acquire the segment lock **before** the semaphore,
matching `doReplaceSegment`. In `BaseTableDataManager.java:1106-1108`:
```java
// current
_segmentReloadSemaphore.acquire(segmentName, _logger);
Lock segmentLock = getSegmentLock(segmentName);
segmentLock.lock();
// proposed
Lock segmentLock = getSegmentLock(segmentName);
segmentLock.lock();
_segmentReloadSemaphore.acquire(segmentName, _logger);
```
with the `finally` block at `:1221-1224` swapped to match:
```java
} finally {
_segmentReloadSemaphore.release();
segmentLock.unlock();
}
```
One wrinkle: `acquire` throws `InterruptedException`, so it needs to move
inside the existing `try` (or get its own try/finally) to guarantee the lock is
released if the acquire is interrupted.
Converging the other way — refresh taking the permit before the lock — also
removes the cycle, but is a worse trade: both paths could then hold the single
global permit while blocked on a fine-grained lock, converting a per-segment
stall into a server-wide one. Holding the segment lock while throttled only
delays that one segment.
Better still would be a single helper that both paths call, so the ordering
cannot diverge again. The bug exists because the same two-resource protocol is
implemented independently in two places.
Worth considering alongside:
- Release the semaphore after download/untar and before index construction.
It exists to bound concurrent segment *loads*, and there is already a separate
instance-level download semaphore for deep-store throttling — in the trace
above the permit was held 20 minutes for 12 seconds of I/O.
- Scope the semaphore per table, or at least document that the default of 1
makes any slow refresh a server-wide stall.
- Timed `tryAcquire`/`tryLock` that fail the Helix message so it surfaces as
ERROR and the controller can retry, rather than hanging indefinitely.
- A gauge for semaphore queue depth and permit hold time. Both values are
already printed in the existing log line (`queue-length`, `lock-time`) but are
not exposed as metrics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
