[Do not Merge] Skip channel lock when channel is active#137
Conversation
adasari
force-pushed
the
adasari/lock-wait
branch
from
1d7acc9 to
b397e9d
Compare
|
Nice finding! Thanks for proposing the improvement. Just curious if we also have any cpu profiles from the time you observed this problem ? |
|
Can we please add tests ? [Code Review via Claude Code] PR #137 Review — Skip channel lock when channel is activeOverviewThe PR addresses broker-to-server CorrectnessThe core approach is sound. This is a classic "optimistic check / lock on miss" pattern. It is valid here because:
Potential concern — window between Channel channel = ensureConnected(timeoutMs); // channel is active here
// ... channel could go inactive here ...
sendRequestWithoutLocking(channel, ...); // writes to possibly dead channelIf the channel dies in this window, Issues1. Stale field comment (line 156) // lock to protect channel as requests must be written into channel sequentially
final ReentrantLock _channelLock = new ReentrantLock();This comment is now wrong. The lock does not serialize writes — Netty channels accept concurrent writes. It serializes the reconnect sequence. Should be updated to reflect the new semantics, e.g.: // Lock to serialize reconnect: prevents two threads from simultaneously seeing _channel == null
// and each initiating a TCP reconnect. Normal sends bypass this lock entirely.2. if (_channelLock.tryLock(timeoutMs, TimeUnit.MILLISECONDS)) {
long lockTimeoutMs = Math.min(timeoutMs, TRY_CONNECT_CHANNEL_LOCK_TIMEOUT_MS);
if (_channelLock.tryLock(lockTimeoutMs, TimeUnit.MILLISECONDS)) {This is pre-existing behavior, but now that the slow path is a distinct code path it's more visible. 3. Write failure not surfaced to the query In channel.writeAndFlush(...).addListener(f -> {
// f.isSuccess() never checked
asyncQueryResponse.markRequestSent(serverRoutingInstance, requestSentLatencyMs);
});If the channel dies right after the fast-path check, the write fails silently and the query eventually times out without a clear error. Consider checking 4. No tests PR notes "Testing: (Pending)". At minimum, a concurrent test that spawns N threads hitting
Minor
Summary
|
No. High SubmitDelayMs noticed from logs. |
adasari
force-pushed
the
adasari/lock-wait
branch
from
01b5c47 to
e78fd42
Compare
adasari
force-pushed
the
adasari/lock-wait
branch
from
e0379cf to
f7a2f64
Compare
2 participants
Under high query concurrency, broker queries to PremiumProfileEngagements (and similar large tables) exhibited SubmitDelayMs values of 126–174ms across 30 servers, contributing to a total query latency of 503ms against a table with 940 segments. See broker log below.
Root cause:
ServerChannel maintains a single Netty Channel per server, protected by a ReentrantLock
The lock is necessary to make the check-reconnect-write sequence atomic i.e two threads must not both see _channel == null and simultaneously attempt a TCP reconnect.
Potential reasons for high SubmitDelayMs:
Improving sequential scatter loop is out of scope.
Options evaluated for the Lock:
Option A has been implemented in this PR, as reconnects are rare events and this approach is more efficient than using a ReadWriteLock under such conditions.
Future enhancements:
Benchmark results and Summary (Generated using Claude):
Setup
Raw Results
alwaysLockvolatileCheckalwaysLockMixed(group total)alwaysLockMixed— senderalwaysLockMixed— disconnectorvolatileCheckMixed(group total)volatileCheckMixed— sendervolatileCheckMixed— disconnectorHead-to-Head Comparison
alwaysLock(ops/s)volatileCheck(ops/s)Stability (Coefficient of Variation)
alwaysLockalwaysLockMixed— senderalwaysLockMixed— disconnectorvolatileCheckvolatileCheckMixed— sendervolatileCheckMixed— disconnectorSummary
Hot Path (~5,089x improvement)
When the channel stays active (steady-state production traffic), the optimized path replaces a
ReentrantLock.lock()/unlock()pair on everysendRequestcall with a singlevolatilefieldread. Under 8-thread contention, lock acquisition requires a CAS loop and full memory barrier —
extremely expensive relative to the actual write work. The volatile read costs nearly nothing by
comparison, yielding a ~5,000x throughput gain.
Mixed Scenario (~1,896x sender improvement)
With 7 concurrent senders and 1 thread simulating periodic disconnects, senders still take the
fast path the vast majority of the time (channel is active between reconnects). Sender throughput
improves by ~1,896x. A secondary benefit: because senders no longer compete for the lock,
the disconnector itself faces far less contention and its throughput rises ~214x as a side effect,
even though its code path is unchanged.
Variance
alwaysLockvariants are highly stable (CV < 6%) — lock serialization naturally smooths variance.volatileCheckMixeddisconnector shows elevated variance (CV ~39%) because each reconnect brieflycauses a burst of senders to fall through to the slow path, creating a short-lived lock storm.
In production this is negligible given the low frequency of actual channel reconnects.
Testing: