Skip to content

Commit 387fe91

Browse files
committed
review
1 parent 1d61de7 commit 387fe91

4 files changed

Lines changed: 439 additions & 86 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1868,8 +1868,14 @@ private WebSocketClient buildAndConnect() {
18681868
// is a worse outcome than a loud connect-time failure.
18691869
if (requestDurableAck && !newClient.isServerDurableAckEnabled()) {
18701870
newClient.close();
1871+
// The "WebSocket upgrade failed:" prefix is load-bearing: the cursor I/O
1872+
// loop's isTerminalUpgradeError() sniffs for that exact substring to
1873+
// classify a connect-time throw as terminal (won't retry). Without the
1874+
// prefix the loop would treat this misconfig as transient and burn the
1875+
// full reconnect budget before surfacing it.
18711876
throw new LineSenderException(
1872-
"server does not support durable ack [host=" + host + ", port=" + port
1877+
"WebSocket upgrade failed: server does not support durable ack [host="
1878+
+ host + ", port=" + port
18731879
+ "]. The client opted in via request_durable_ack=on but the server "
18741880
+ "did not echo X-QWP-Durable-Ack: enabled in the upgrade response. "
18751881
+ "Either disable request_durable_ack or connect to a server with "
@@ -1950,7 +1956,7 @@ private void drainOnClose() {
19501956
cursorSendLoop.checkError();
19511957
if (System.nanoTime() >= deadlineNanos) {
19521958
long acked = cursorEngine.ackedFsn();
1953-
LOG.warn("close() drain timed out after {}ms [target={} acked={}] pending data may be lost",
1959+
LOG.warn("close() drain timed out after {}ms [target={} acked={}], pending data may be lost",
19541960
closeFlushTimeoutMillis, target, acked);
19551961
throw new LineSenderException("close() drain timed out after ")
19561962
.put(closeFlushTimeoutMillis).put(" ms [publishedFsn=")

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -794,7 +794,7 @@ public static WebSocketClient connectWithRetry(
794794
} catch (Throwable e) {
795795
if (isTerminalUpgradeError(e)) {
796796
String upgradeMsg = findUpgradeFailureMessage(e);
797-
LOG.error("{} hit terminal upgrade error won't retry: {}",
797+
LOG.error("{} hit terminal upgrade error, won't retry: {}",
798798
contextLabel, upgradeMsg);
799799
throw new LineSenderException(
800800
"WebSocket upgrade failed during " + contextLabel
@@ -1098,7 +1098,7 @@ public void onBinaryMessage(long payloadPtr, int payloadLen) {
10981098
if (highestSent < 0) return; // ACK before any send — ignore
10991099
long capped = Math.min(wireSeq, highestSent);
11001100
if (capped < wireSeq) {
1101-
LOG.warn("server ACK wire seq {} exceeds highest sent {} clamping",
1101+
LOG.warn("server ACK wire seq {} exceeds highest sent {}, clamping",
11021102
wireSeq, highestSent);
11031103
}
11041104
totalAcks.incrementAndGet();
@@ -1143,6 +1143,45 @@ public void onBinaryMessage(long payloadPtr, int payloadLen) {
11431143
handleServerRejection(wireSeq);
11441144
}
11451145

1146+
private void handlePreSendRejection(long wireSeq, byte status,
1147+
SenderError.Category category,
1148+
SenderError.Policy policy) {
1149+
LOG.warn("server rejection wire seq {} (category={}, status=0x{}) before any send -- skipping ack advance",
1150+
wireSeq, category, Integer.toHexString(status & 0xFF));
1151+
// Use the same [ackedFsn+1, publishedFsn] span the
1152+
// protocol-violation close path uses (see onClose above): there
1153+
// is no FSN we can attribute the rejection to, so we report
1154+
// the unacked range the producer can correlate against.
1155+
long fromFsn = engine.ackedFsn() + 1L;
1156+
long toFsn = Math.max(fromFsn, engine.publishedFsn());
1157+
String tableName = response.getTableEntryCount() == 1
1158+
? response.getTableName(0)
1159+
: null;
1160+
SenderError err = new SenderError(
1161+
category,
1162+
policy,
1163+
status & 0xFF,
1164+
response.getErrorMessage(),
1165+
wireSeq,
1166+
fromFsn,
1167+
toFsn,
1168+
tableName,
1169+
System.nanoTime()
1170+
);
1171+
totalServerErrors.incrementAndGet();
1172+
if (policy == SenderError.Policy.HALT) {
1173+
// Latch the typed terminal error before invoking the handler
1174+
// so a synchronous probe of getLastTerminalError() / flush()
1175+
// from inside the handler observes the typed error. Mirrors
1176+
// the ordering in the post-send HALT path below.
1177+
recordFatal(new LineSenderServerException(err), err);
1178+
}
1179+
// DROP_AND_CONTINUE: no watermark advance -- there is nothing
1180+
// sent on this connection to drop. The dispatch is the user's
1181+
// only handle to the server's complaint.
1182+
dispatchError(err);
1183+
}
1184+
11461185
private void handleServerRejection(long wireSeq) {
11471186
byte status = response.getStatus();
11481187
SenderError.Category category = classify(status);
@@ -1153,9 +1192,23 @@ private void handleServerRejection(long wireSeq) {
11531192
// makes the segment manager trim sealed segments the I/O thread
11541193
// is still reading — and the next Unsafe.getInt SEGVs the JVM.
11551194
long highestSent = nextWireSeq - 1L;
1195+
if (highestSent < 0L) {
1196+
// Pre-send rejection: server emitted an error frame before
1197+
// we sent anything on this connection (typical after a
1198+
// fresh swapClient — auth failure, server-initiated halt,
1199+
// etc.). The server-named wireSeq does not correspond to
1200+
// any frame we sent, so clamping it to 0 and acknowledging
1201+
// fsnAtZero would silently advance ackedFsn past a real
1202+
// unsent batch (fsnAtZero == ackedFsn + 1 right after a
1203+
// swap). Skip the watermark advance entirely; still surface
1204+
// the error so the user's handler sees it and HALT errors
1205+
// remain producer-observable.
1206+
handlePreSendRejection(wireSeq, status, category, policy);
1207+
return;
1208+
}
11561209
long cappedSeq = Math.max(0L, Math.min(wireSeq, highestSent));
11571210
if (cappedSeq < wireSeq) {
1158-
LOG.warn("server NACK wire seq {} exceeds highest sent {} clamping",
1211+
LOG.warn("server NACK wire seq {} exceeds highest sent {}, clamping",
11591212
wireSeq, highestSent);
11601213
}
11611214
long fsn = fsnAtZero + cappedSeq;

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SenderProgressDispatcher.java

Lines changed: 80 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -29,32 +29,32 @@
2929
import org.slf4j.Logger;
3030
import org.slf4j.LoggerFactory;
3131

32-
import java.util.concurrent.LinkedBlockingDeque;
33-
import java.util.concurrent.TimeUnit;
3432
import java.util.concurrent.atomic.AtomicLong;
33+
import java.util.concurrent.locks.LockSupport;
3534

3635
/**
37-
* Bounded inbox + lazy-started daemon thread that delivers ack-watermark
38-
* advances to a user-supplied {@link SenderProgressHandler} off the I/O thread.
36+
* Single-slot watermark mailbox + lazy-started daemon thread that delivers
37+
* ack-watermark advances to a user-supplied {@link SenderProgressHandler} off
38+
* the I/O thread.
3939
*
4040
* <h2>Why a separate thread</h2>
4141
* The I/O thread must never block on user code. The same rationale as the
4242
* {@link SenderErrorDispatcher} sibling: a slow handler (e.g. journal write)
4343
* cannot stall send progress. The I/O thread {@link #offer offers} the new
44-
* watermark and continues; the daemon dispatcher takes from the queue and
44+
* watermark and continues; the daemon dispatcher takes the latest value and
4545
* invokes the handler.
4646
*
47-
* <h2>Why drop-on-overflow is safe here</h2>
48-
* Watermarks are <em>monotonically increasing</em>. If the inbox is full and an
49-
* offer is dropped, the next successful offer carries a higher (or equal) FSN
50-
* — so any handler watching for "ackedFsn &gt;= target" still catches up. This
51-
* is the key reason a bounded queue is acceptable for what is otherwise a
52-
* high-volume signal: drops compress, they don't lose information.
47+
* <h2>Why a single-slot mailbox is sufficient</h2>
48+
* Watermarks are <em>monotonically increasing</em>. If the handler is busy
49+
* when several offers arrive in succession, only the highest watermark is
50+
* worth delivering — earlier advances are subsumed by it. The mailbox is
51+
* therefore a single {@code volatile long} that the producer overwrites
52+
* unconditionally; the consumer reads it and delivers anything strictly
53+
* greater than its previous delivery.
5354
*
54-
* <p>Implementation detail: a {@link LinkedBlockingDeque} is used so on full
55-
* inbox we drop the <em>oldest</em> entry rather than the newest. The freshest
56-
* watermark is the one the handler cares about, and a sustained burst should
57-
* not bury it behind stale values.
55+
* <p>This collapses a high-volume signal to a constant memory footprint and
56+
* keeps the I/O thread on a zero-allocation hot path: no autoboxing, no
57+
* per-offer node allocation, no {@code java.util.*} structures.
5858
*
5959
* <h2>Lifecycle</h2>
6060
* The dispatcher thread is started lazily on the first successful
@@ -69,24 +69,30 @@ public final class SenderProgressDispatcher implements QuietCloseable {
6969

7070
public static final int DEFAULT_CAPACITY = 256;
7171
private static final long DRAIN_DEADLINE_NANOS = 100_000_000L; // 100 ms
72+
// Park interval used when the mailbox is idle. Bounded so a missed unpark
73+
// (e.g. from a future code path) cannot wedge the dispatcher.
74+
private static final long IDLE_PARK_NANOS = 100_000_000L; // 100 ms
7275
private static final Logger LOG = LoggerFactory.getLogger(SenderProgressDispatcher.class);
73-
// Sentinel pushed during close() to nudge the dispatcher out of poll().
74-
// Identity-compared in the loop body; never delivered to the handler.
75-
// Box the sentinel so it has reference identity distinct from any real
76-
// value the I/O thread might offer.
77-
private static final Long POISON = Long.MIN_VALUE;
78-
private final AtomicLong dropped = new AtomicLong();
76+
// Sentinel for "no value ever offered". Real FSNs are non-negative, so
77+
// Long.MIN_VALUE is unambiguous and the dispatcher's "deliver if greater
78+
// than lastDelivered" check naturally skips it.
79+
private static final long EMPTY = Long.MIN_VALUE;
7980
// volatile so the producer of progress events can swap the handler
8081
// post-connect. Each delivery reads the current reference; concurrent
8182
// updates may interleave a final-old / first-new delivery, which is
8283
// acceptable for the watermark contract (monotonic + idempotent).
8384
private volatile SenderProgressHandler handler;
84-
private final LinkedBlockingDeque<Long> inbox;
8585
private final Object lock = new Object();
8686
private final String threadName;
8787
private final AtomicLong totalDelivered = new AtomicLong();
88+
private final AtomicLong totalOffered = new AtomicLong();
8889
private volatile boolean closed;
8990
private Thread dispatcherThread;
91+
// Single-slot inbox. Producer (I/O thread) overwrites unconditionally;
92+
// consumer reads via volatile and skips when the value has already been
93+
// delivered. Monotonic FSNs make overwrite safe -- the latest value
94+
// subsumes any prior undelivered value.
95+
private volatile long latestFsn = EMPTY;
9096

9197
public SenderProgressDispatcher(SenderProgressHandler handler, int capacity) {
9298
this(handler, capacity, "qdb-sf-progress-dispatcher");
@@ -96,11 +102,13 @@ public SenderProgressDispatcher(SenderProgressHandler handler, int capacity, Str
96102
if (handler == null) {
97103
throw new IllegalArgumentException("handler must be non-null");
98104
}
105+
// Capacity is retained on the constructor for API compatibility but
106+
// the mailbox is single-slot by design (see class doc). The bound is
107+
// still validated so misconfiguration surfaces at construction.
99108
if (capacity < 1) {
100109
throw new IllegalArgumentException("capacity must be >= 1, was " + capacity);
101110
}
102111
this.handler = handler;
103-
this.inbox = new LinkedBlockingDeque<>(capacity);
104112
this.threadName = threadName;
105113
}
106114

@@ -111,9 +119,11 @@ public void close() {
111119
return;
112120
}
113121
closed = true;
114-
inbox.offer(POISON);
115122
Thread t = dispatcherThread;
116123
if (t != null) {
124+
// Wake the dispatcher in case it is parked. After observing
125+
// closed=true with no pending value, the loop returns.
126+
LockSupport.unpark(t);
117127
long deadline = System.nanoTime() + DRAIN_DEADLINE_NANOS;
118128
long remainingMillis;
119129
while ((remainingMillis = (deadline - System.nanoTime()) / 1_000_000L) > 0) {
@@ -126,7 +136,7 @@ public void close() {
126136
}
127137
if (t.isAlive()) {
128138
LOG.warn("progress-dispatcher thread did not exit within drain deadline; "
129-
+ "abandoning {} queued advances", inbox.size());
139+
+ "abandoning latest pending advance");
130140
t.interrupt();
131141
}
132142
dispatcherThread = null;
@@ -135,13 +145,17 @@ public void close() {
135145
}
136146

137147
/**
138-
* Total advances dropped since startup due to inbox-overflow. Non-zero
139-
* means the user's handler is slower than the ack rate — typically not a
140-
* correctness issue (later advances catch up), but useful as an ops signal
141-
* when handler latency matters.
148+
* Watermark advances overwritten before the handler observed them, since
149+
* startup. Computed as {@code totalOffered - totalDelivered}; a small
150+
* non-zero value typically reflects a single in-flight advance, while a
151+
* persistently growing value signals a handler slower than the ack rate.
152+
* Useful as an ops signal when handler latency matters.
142153
*/
143154
public long getDroppedNotifications() {
144-
return dropped.get();
155+
long offered = totalOffered.get();
156+
long delivered = totalDelivered.get();
157+
long diff = offered - delivered;
158+
return diff > 0L ? diff : 0L;
145159
}
146160

147161
/**
@@ -164,72 +178,57 @@ public void setHandler(SenderProgressHandler handler) {
164178

165179
/**
166180
* Non-blocking enqueue of an ack-watermark advance. Returns {@code true}
167-
* if the value will be delivered (eventually, on the dispatcher thread).
168-
* Returns {@code false} if the dispatcher was closed.
181+
* if the value will be visible to the dispatcher (eventually delivered,
182+
* possibly subsumed by a later advance). Returns {@code false} if the
183+
* dispatcher was closed.
169184
*
170-
* <p>On a full inbox this method evicts the oldest queued value to make
171-
* room for the new one — the freshest watermark is what observers want.
172-
* The dropped-old count is bumped for ops visibility.
173-
*
174-
* <p>Lazy-starts the dispatcher thread on the first successful offer.
185+
* <p>Watermarks are monotonic, so the slot is overwritten unconditionally:
186+
* a later advance subsumes any earlier one not yet observed by the
187+
* handler. Lazy-starts the dispatcher thread on the first offer.
175188
*/
176189
public boolean offer(long ackedFsn) {
177190
if (closed) {
178191
return false;
179192
}
180-
// Try the fast path first: a non-full inbox accepts immediately.
181-
if (inbox.offerLast(ackedFsn)) {
182-
if (dispatcherThread == null) {
183-
startDispatcherIfNeeded();
184-
}
185-
return true;
186-
}
187-
// Inbox full: evict the oldest entry and try again. We do this rather
188-
// than dropping the newest so the handler always sees the most recent
189-
// watermark even under sustained backpressure.
190-
inbox.pollFirst();
191-
dropped.incrementAndGet();
192-
boolean accepted = inbox.offerLast(ackedFsn);
193-
if (accepted && dispatcherThread == null) {
193+
totalOffered.incrementAndGet();
194+
latestFsn = ackedFsn;
195+
Thread t = dispatcherThread;
196+
if (t == null) {
194197
startDispatcherIfNeeded();
198+
t = dispatcherThread;
195199
}
196-
return accepted;
200+
if (t != null) {
201+
LockSupport.unpark(t);
202+
}
203+
return true;
197204
}
198205

199206
private void dispatchLoop() {
200-
// Local high-water filter: skip any queued value <= the last one we
201-
// already delivered, in case multiple advances queued while we were
202-
// running the handler. The contract guarantees the user only sees
203-
// strictly-increasing values.
204-
long lastDelivered = Long.MIN_VALUE;
205-
while (!closed || !inbox.isEmpty()) {
206-
Long v;
207-
try {
208-
v = inbox.poll(100, TimeUnit.MILLISECONDS);
209-
} catch (InterruptedException e) {
210-
if (closed) {
211-
return;
207+
// Strictly-monotonic delivery: skip the slot when its value is not
208+
// greater than the last value handed to the handler.
209+
long lastDelivered = EMPTY;
210+
while (true) {
211+
long v = latestFsn;
212+
if (v > lastDelivered) {
213+
lastDelivered = v;
214+
// Increment before invoking the handler so observers using a
215+
// CountDownLatch in the handler can read the updated counter
216+
// once their latch fires.
217+
totalDelivered.incrementAndGet();
218+
SenderProgressHandler h = handler;
219+
try {
220+
h.onAcked(v);
221+
} catch (Throwable t) {
222+
LOG.error("SenderProgressHandler threw on fsn={}: {}", v, t.getMessage(), t);
212223
}
213-
Thread.currentThread().interrupt();
214-
continue;
215-
}
216-
if (v == null || v.equals(POISON)) {
217-
continue;
218-
}
219-
long fsn = v;
220-
if (fsn <= lastDelivered) {
221224
continue;
222225
}
223-
lastDelivered = fsn;
224-
// Increment before invoking the handler so observers using a
225-
// CountDownLatch in the handler can read the updated counter
226-
// once their latch fires.
227-
totalDelivered.incrementAndGet();
228-
try {
229-
handler.onAcked(fsn);
230-
} catch (Throwable t) {
231-
LOG.error("SenderProgressHandler threw on fsn={}: {}", fsn, t.getMessage(), t);
226+
if (closed) {
227+
return;
232228
}
229+
// No new value; park until the producer unparks us or the idle
230+
// tick fires. Spurious wakeups loop back to the top and re-check.
231+
LockSupport.parkNanos(this, IDLE_PARK_NANOS);
233232
}
234233
}
235234

0 commit comments

Comments
 (0)