Repository navigation
Conversation
jovfer
marked this pull request as ready for review
October 14, 2026 18:21
…s to Sender Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…_on_close restores the old path Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ure test, stronger assertions Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…nal lease Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ettle slots Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lows auto_flush_interval Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…connect-string number syntax Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…he settle cadence Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…at relied on close flushing Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ent the defaults Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…window, settle commits idle groups Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ten close() docs Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…flush_on_close=on Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… string window keys Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…minal and retriable rejections Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…urces keeps the primary exception close() threw the delegate's latched instance, which a lease body may already have thrown; try-with-resources then failed with a self-suppression IllegalArgumentException. close() now throws a new LineSenderException with the original as cause. Tests updated, terminal test restored to try-with-resources and hardened. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rs; tolerate a late-latched terminal in the test Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… mark on drain, typed close errors Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…skips the mark With a window of 2 the close right after the explicit drain or flush reached the threshold on its own, performed a no-op committing flush and marked the slot, so the tests passed with the mark removed from the pooled drain() and flush(). Use a window of 3, so the stale counter would commit on the third row, and assert where the window closes after the reset. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
jovfer
force-pushed
the
sm_pool_flush_on_close
branch
from
October 15, 2026 11:11
e8c74ec to
0b2d5ba
Compare
…_flush_bytes is unset The WebSocket config-string default for auto_flush_bytes was 0, which the clamp read as an explicit opt-out, so nothing capped a batch's bytes. With cross-lease accumulation in the pool, 1000 wide rows exceed the server's cap and the batch cannot ship. Unset now means AUTO_FLUSH_BYTES_SERVER_CAP: no byte trigger before the handshake, 90 percent of the advertised cap after it, none on a node that advertises no cap. An explicit off stays off. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…to it rollbackToSavepoint truncates every column to the row count recorded by markSavepoint and removes the columns added since, reusing the per-column truncateTo that cancelCurrentRow already relies on. reset moves the mark to the empty buffer, clear drops it. Groundwork for a lease-scoped reset on the pooled sender. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…eset does A flush that keeps a row in progress ships the committed rows and sets rowCount to 0; leaving the savepoint where it was would make a later rollback truncate to rows that were already shipped. Also states the never-marked case in the rollback Javadoc. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ion; tests for the retain and never-marked cases truncateTo clears geohashPrecision when it leaves a column empty, which is right for a cancelled first row but wrong for a server-known column a lease rolls back: the server has locked that precision. The rollback now restores it for every column below the mark. Adds the column half of the retainInProgressRow test, the never-marked rollback test that a table created after the mark relies on, and moves the new members to their alphabetical positions. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…eports a retained over-cap batch resetToSavepoint rolls every table buffer back to the row and column counts recorded by markSavepoint and restores the pending counters; with an empty savepoint it is reset(). A flush that ships the pending rows empties the savepoint. flushPendingRows remembers a BatchTooLargeForCapException until reset, rollback or a successful flush clears it, and checkError throws it, so a pool can tell a slot whose batch will never ship from a healthy one. SenderProbeMethodsTest now has its test methods in alphabetical order and uses QuestDBFlushOnCloseTest.wideValue(1000) instead of a hand-rolled char array. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…sent column definitions Rows a lease rolls back may have raised currentBatchMaxSymbolId; keeping that value made the retained rows re-encode the dropped rows' dictionary entries, so a dictionary section the cap rejected was rejected again on the next borrower after the probe had been cleared. The mark now records the watermark and the rollback restores it: rows before the mark reference only ids at or below it. The empty-savepoint branch used reset(), which keeps column definitions the lease created while the rollback branch removes them; resetToSavepoint now rolls back every buffer in both branches, so a column the server never saw does not ship null-padded to it. Tests cover the single-symbol latch and the pool's mark-flush-rollback-ship case. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…egistered Restoring the batch watermark kept the retained rows from re-encoding the dropped rows' dictionary entries, but the entries stayed in the dictionary, so the next row with a new symbol re-shipped the whole orphaned range and an over-cap dictionary section came back on a later lease with the probe clean. In delta mode the non-empty branch now reclaims ids above the restored watermark, the sent watermark and the persisted dictionary, through the existing reclaim helper. A test drops six wide symbols, flushes the retained row and then adds a new symbol. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
With flush_on_close=off a slot buffer holds rows from earlier leases whose close() already returned. The delegate's reset() cleared every table buffer, so a borrower following the BatchTooLargeForCapException remedy silently lost those rows. borrow() now marks a savepoint on the delegate and the pooled reset() rolls back to it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A settle flush that throws BatchTooLargeForCapException used to requeue the slot: the probe did not see the rejection, so the housekeeper retried every tick and later borrowers failed on their own appends. The settle now retires the slot and logs the dropped row count; at close the probe reports the retained batch, so the release retires the slot and the wrapper names the rows it dropped. borrow() now also marks a savepoint explicitly on a freshly created delegate, so every lease handed to a caller starts at a mark. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… not dropped, in the close wrapper The helper landed between hasUnshippedWork's Javadoc and its declaration. The close wrapper's count is read before the delegate closes, and a transient failure can still ship those rows from the delegate's close, so the message now says unshipped; the settle WARN keeps "dropping" because the over-cap path discards for real. Comments and Javadoc no longer claim the rejection is unlatched or that no node could ship it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ekeeper deadlines The two-deadline loop retried a failed recovery step on the settle cadence (about 100 ms) instead of the 5 s reap interval, and computed now + interval without overflow protection, so housekeeper_interval_ms=Long.MAX_VALUE made the loop wait forever and the settle never ran. Recovery retries now have their own one-second deadline, matching the direct pool's driver, and every deadline saturates. Drops the unused parameter of resolveSettleIntervalMillis and corrects the constructor Javadoc. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…etical positions deadline() had landed between joinIgnoringCallerInterrupts and its Javadoc, leaving that method undocumented and a dangling comment above the helper. Also moves the retry constant and the test accessor into order and corrects two comments. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…he retryable wrapper and the settle Error path Each of the four tests fails when its line is mutated: markCommitted without the age reset, flushAndGetSequence without the mark, a hard-coded retryable=false on the close wrapper, and a settle Error that requeues instead of retiring. testResetAfterAFlushInsideTheLeaseDropsEveryPendingRow additionally pins the savepointRows reset in RecordingSender.flush(). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…y; fix the examples; license headers The README said a rejected frame makes the client replay the healthy frames. It does not: a terminal halts the slot and a retriable replays the whole frame. The pool docs now say that one frame carries several leases' rows and name flush() and flush_on_close=on as the isolation tools. The examples no longer claim close() flushes. A test pins the documented behaviour on the wire. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…; wrap two long lines WsExample still said that close() flushes pending rows, which is false with the default flush_on_close=off and contradicted the class Javadoc above it. The README's pooled-ingest example made the same claim in a comment. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…arlier leases' rows down with it The byte trigger fires after the crossing row is committed, so a row larger than the 10 percent margin makes the auto-flush reject a batch that is mostly other leases' rows. The release path used to retire the slot on the retained rejection and drop them all. It now rolls the lease back to its borrow mark, retries the flush, keeps the slot when the inherited rows ship, and retires only a batch the cap still rejects. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s log at ERROR, savepoint Javadoc for pooled senders
BatchTooLargeForCapException now separates its detail from the standalone
remedy ("call reset() and keep this sender"), and the pool's close wrapper
and settle log use the detail only. Slot retirements that drop rows log at
ERROR, the level the default error handler uses for data loss. The Sender
Javadoc states that a pooled sender ignores markSavepoint() and that
resetToSavepoint() on it is the lease-scoped reset(). The retained
rejection is cleared once a later flush attempt passes the pre-flight, the
housekeeper reuses the pool's one-second recovery constant, and the
cap-bound default documents the pre-handshake window.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
[PR Coverage check]😍 pass : 420 / 449 (93.54%) file detail
|
This branch has not been deployed
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.
Why
Applications that borrow a pooled
Senderper message paid one QWP frame and one server-side WAL commit per message, because the pooledclose()flushed. A customer benchmark of that shape took 54 s for 1M rows against 6.5 s for the batched HTTP sender, and the server's commit rate was the ceiling. The quick start taught exactly that shape.What changes
Close releases, flush ships, drain waits.
close()on a borrowed sender now returns the slot and ships nothing by itself. Rows go out when an auto-flush threshold trips on a later append, or when the pool's housekeeper settles the idle slot on theauto_flush_intervalcadence (100 ms by default). Borrowing per message therefore batches exactly like a sender held for the whole run.flush()on a lease still ships now,drain()still waits for the server.flush_on_close=on(builderflushOnClose(true)) restores the pre-1.4 behaviour: one frame and one commit per close.housekeeper_interval_ms. A slot nobody borrows again no longer keeps its last rows until the handle closes.close()is cancelled and the slot kept (the close still throws); a dead delegate is retired by an explicit probe instead of a throwing flush.LineSenderServerExceptionkeeps its type, other errors arrive as aLineSenderExceptionwhose cause is the delegate's error. A lease body that already threw keeps its exception as the primary one (previously try-with-resources failed with "Self-suppression not permitted").Sendergains eight default methods used by the pool:checkError,hasRowInProgress,pendingRowCount,rowsAppended,flushDeferred,hasDeferredFrames,markSavepoint,resetToSavepoint.Commit window. With
flush_on_close=on,commit_rowsandcommit_interval_ms(buildercommitRows,commitIntervalMillis; setting one defaults the other to 1000 rows / 100 ms) make each close ship a frame flagged defer-commit and the pool commit once per window: the release that fills the window ships the committing frame, and the settle commits an idle open group. Rows are in the store-and-forward ring the momentclose()returns, and the server commits once per window instead of once per close. Acks arrive at the commit, so adrain()should follow aflush(). The builder injectstransaction=onfor this mode and rejects a window withflush_on_close=off(designed, gated until measured) or withtransaction=off.Wording trap. In this client "flush" means published to the store-and-forward ring, not acknowledged by the server.
flush_on_close=ondoes not mean committed whenclose()returns.Review round 1 fixes
reset().borrow()marks a savepoint on the delegate and the pooledreset()rolls back to it, so a borrower following theBatchTooLargeForCapExceptionremedy no longer discards rows that earlier leases left in the slot. The WebSocket sender rolls every table buffer back to the mark, restores the pending counters and the batch symbol watermark, reclaims the dictionary ids the dropped rows registered (delta mode), and drops column definitions the server has not seen. A pooled sender ignores a borrower'smarkSavepoint(); itsresetToSavepoint()is the lease-scopedreset().auto_flush_bytesunset, the WebSocket sender now flushes at 90 percent of the batch cap the server advertises; an explicitoffkeeps byte flushing off, and a node that advertises no cap gets no byte trigger. This changes the standalone default too: a batch the server would reject ships earlier instead. Rows appended before the first handshake (async connect) carry no byte trigger; the first append after it flushes the backlog.reset(), a rollback or a later flush that passes the pre-flight check, and reports it throughcheckError(). At release the pool first rolls the lease back to its borrow mark and retries the flush: when the rows earlier leases left ship, the slot stays in service and the borrower gets a wrapper naming its own dropped rows. Only an inherited batch the cap still rejects retires the slot. The settle step retires such a slot directly. Both retirements log at ERROR with the dropped row count, and the pool's messages no longer repeat the standalone "call reset() and keep this sender" remedy.housekeeper_interval_ms=Long.MAX_VALUEdisables reaping and nothing else.flush()beforeclose()andflush_on_close=onas the isolation tools. Example and README comments that saidclose()flushes are corrected; license headers added to two test classes.Trade-off
With the default, a terminal rejection of one lease's row discards the other leases' rows in the same frame, after their
close()calls returned, and the error surfaces on a later call on that slot, possibly to a lease that wrote none of the rejected rows. This is the HTTP sender's failure span moved across threads. The alternative,flush_on_close=onas the default, keeps one frame and one commit per message, which is the regression this PR removes.The byte trigger measures raw column bytes after the row that crosses it is committed, so a row larger than the remaining 10 percent can still push a batch over the cap. The release path above limits the loss to that lease's rows; the settle path, where no lease owns the batch, still retires the slot.
One existing test with a 150-byte advertised cap needed
auto_flush_bytes=offto keep its explicit-flush control flow. Downstream tests that callsetAdvertisedMaxBatchSizewith small values may need the same.Benchmark
Manual benchmark (
PoolBorrowPerRowBenchmark,@Ignored), 1M rows against the in-repo ack-all test server, which has no WAL:flush_on_close=on(previous behaviour)flush_on_close=on+ window 1000/100The frame count dominates on the client and the wire. The window's commit saving needs a real server to show; the test server commits nothing.
Release note (1.4.0)
close()on a pooled sender no longer flushes; setflush_on_close=onto restore it. Rows left in a slot ship withinauto_flush_interval. Close-time errors are wrapped with the delegate's error as cause. New keyscommit_rowsandcommit_interval_ms.reset()on a pooled sender discards only the rows appended through that lease. Withauto_flush_bytesunset, the WebSocket sender flushes at 90 percent of the server's advertised batch cap.Follow-ups
QuestDBFacadeE2ETest#testTwoSendersPerTableWhileReaderThreadStreamsTwoQueriesrelies on close-time flushing with a one-hourauto_flush_interval; it needsflush()before close orflush_on_close=onwhen the submodule pin moves to this release.borrowSender()(QwpRoleBounceChaosLosslessTest,SqlFailoverQwpDeferredCloseExactlyOnceTest,QwpSidecarMain,QwpSidecarMainTest) and need the same check.SenderErrorHandler; routing client-side drops there needs a client-side error category.flush_on_close=on, with or without a commit window.🤖 Generated with Claude Code
https://claude.ai/code/session_01ENEBF3hLEx585DdJfKoUSG