refactor(qwp): rename sf_max_bytes to sf_max_segment_bytes

[mtopolnik]

Tandems:

questdb/questdb#7413
https://github.com/questdb/questdb-enterprise/pull/1140

Renames the store-and-forward connect-string key sf_max_bytes to sf_max_segment_bytes: the key caps a single SF segment, and the new name says so; it also pairs cleanly with sf_max_total_bytes. The builder method follows: storeAndForwardMaxBytes() is now storeAndForwardMaxSegmentBytes(). Internal identifiers, error messages, and test assertions rename in step, so git grep -iE 'sf[_-]?max[_-]?bytes|storeandforwardmaxbytes' returns zero.

The key shipped only in beta-state releases (1.2.1–1.3.5, not on Maven Central), so this is a hard rename — no alias, no deprecation path.

Part of the cross-repo rename of this key (server, c/rust, go, .NET, python, enterprise, docs land in the same window). This repo sets the naming precedent; the QuestDB server PR bumps its submodule to this branch's commit and should merge right after this one.

Full core test suite is green locally, including the five affected test classes.

Note

The red "Build, test & javadoc (JDK 8)" check is the known MmapSegmentRecoveryFaultTest InternalError flake tracked by #69 — pre-existing #64 code, where C2-inlined async unsafe-fault delivery escapes the converting catch. It reproduced twice on this PR with a different failing test each time, and this diff only touches that code path in one comment line, so it is unrelated to the rename. The fix is #71, which reads the recovery scan via pread instead of the mapping so there is no fault to catch at all — merge #71 first, then rebase/merge this PR to clear the check for good.

🤖 Generated with Claude Code

[mtopolnik]

Review: PR #70

Verdict: Approve. This is a clean, complete, behavior-preserving rename. No Critical or Moderate findings.

What I verified

• Completeness — git grep -iE 'sf[_-]?max[_-]?bytes|storeandforwardmaxbytes' returns zero hits across the whole client repo (the PR body's claim holds), and 32 consistent occurrences of the new names. The case-insensitive pattern also covers the CamelCase field/local (sfMaxBytes, actualSfMaxBytes), so nothing was missed.
• Callsite consistency in build() — all three uses of the renamed local were updated in lockstep: the total-bytes default Math.max(defaultMaxTotal, actualSfMaxSegmentBytes * 2), the CursorSendEngine ctor arg, and startOrphanDrainers(...). Field init to PARAMETER_NOT_SET_EXPLICITLY preserved.
• Error messages — the "invalid …"/"… must be positive" text is threaded from the name argument (parseSizeValue(sink, "sf_max_segment_bytes") → LineSenderException("invalid ").put(name)), so the renamed message and the test assertion (contains("invalid sf_max_segment_bytes")) stay in sync. Both parse paths (legacy fromConfig if/else chain and WS fromConfigWebSocket ConfigView), ConfigSchema INGRESS registration, and wsConfigSnapshotForTest were all renamed.
• Non-production changes are comment-only — MmapSegment.java, SegmentRing.java, CursorWebSocketSendLoop.java, and one SegmentRingTest javadoc are single comment-line edits. The only executable changes are identifier/string renames in Sender.java + one string literal in ConfigSchema.java. No behavioral delta.
• Cross-repo coordination — the parent questdb repo (on the coordinated mt_sf-max-segment-bytes branch) already uses sf_max_segment_bytes in QwpIngressOracleFuzzTest.java with zero old-name references, so the submodule bump won't break the parent build.
• Tests still bite — Sender.fromConfig(...) throws on an unknown key, so the renamed testSfMaxSegmentBytesParsing, assertHonored("sf_max_segment_bytes=4096", ...), and testIngressOnlyKeysSilentlyAcceptedOnEgress exercise the new key through the real parse path and would fail if the rename were incomplete on the ingress side. Assertions are unchanged in substance.
• No binaries in the diff; no Java-8-incompatible constructs introduced.

Minor

• Sender.java:2760 — the public builder method renamed to storeAndForwardMaxSegmentBytes(long maxBytes) but the parameter is still maxBytes. Cosmetic only (no @param javadoc), but for full coherence with the "per segment" semantics it reads better as maxSegmentBytes. The javadoc itself ("Maximum bytes per segment file before rotation") is already correct. Not blocking.
• Hard rename, no alias — a caller still passing the old sf_max_bytes connect-string key now gets a generic unknown-key rejection rather than a "renamed to sf_max_segment_bytes" hint. This is acceptable per project policy (QWP/SF is beta and unreleased; the key shipped only in beta builds 1.2.2–1.3.4, never on Maven Central) and matches the existing accept-union/reject-legacy stance — I'm flagging it only so the choice is explicit, not as a defect.

Summary

• No regressions, no tradeoffs beyond the deliberate hard-rename of a beta key. Ready to merge once the JDK 8 check is green.

[mtopolnik]

Reviewing PR #70 at level 1

The rename itself is provably semantics-free: inverse-renaming the PR-branch Sender.java and ConfigSchema.java back to the old identifiers produces a byte-identical file against the merge base. Defaults, the actualSfMaxSegmentBytes * 2 arithmetic, and argument order into CursorSendEngine(...) / startOrphanDrainers(...) are unchanged. Zero occurrences of sf_max_bytes / sfMaxBytes / storeAndForwardMaxBytes remain anywhere in this repo.

Critical

None.

Moderate

1. No test pins that the old key is now rejected — in-diff omission.
This PR's only user-visible behavior change is that ws::…;sf_max_bytes=… stops working, and nothing asserts it. Every guard that does exist (WsSenderConfigHonoredTest, QwpConfigKeysTest.testEverySchemaKeyIsRecognizedByBothClients) iterates ConfigSchema.all(), so by construction they can only catch a half-done rename, never a whole-registry one. An accidental re-introduction of the alias would go unnoticed.

The helper is already there — LineSenderBuilderWebSocketTest.java:1087 assertBadConfig(String, String...), used at :828, :834, :839, :848. One line:

assertBadConfig("ws::addr=localhost:9000;sf_max_bytes=4096;", "unknown configuration key: sf_max_bytes");

2. ConfigView.RELOCATED_HINTS gets no entry for the removed key — out-of-diff, impl/ConfigView.java:58-68.
Users hitting this get a bare unknown configuration key: sf_max_bytes, versus retry_timeout's unknown configuration key: retry_timeout (use reconnect_max_duration_millis on ws/wss).

The exposure is wider than the PR body states. git tag --contains edbdc3a8 (the commit that introduced the key) returns 1.2.1, 1.2.2, 1.3.0, 1.3.1, 1.3.2, 1.3.3, 1.3.4, 1.3.5 — the body says "1.2.2–1.3.4", missing 1.2.1 at the bottom and 1.3.5 at the top. GitHub releases exist for 1.2.1, 1.2.2, 1.3.0, 1.3.1, 1.3.4.

There is a real charter conflict here, so decide it explicitly rather than by omission: the table's javadoc scopes it to "keys that live in the legacy http/tcp/udp vocabulary", and QwpConfigKeysTest.testRelocatedHintTableIsExactlyTheLegacyKeys:109 is named for that. A renamed-QWP-key hint broadens it. Note that test asserts 8 named keys, "no registry key has a hint", and 3 explicit nulls — it never checks map size, so adding hints.put("sf_max_bytes", "(renamed to sf_max_segment_bytes)") passes silently; add a pin at :112-119 if you take it.

3. Parameter name left stale — in-diff, Sender.java:2778.

public LineSenderBuilder storeAndForwardMaxSegmentBytes(long maxBytes) {

Every sibling names its parameter after the method tail: storeAndForwardMaxTotalBytes(long maxTotalBytes) :2798, storeAndForwardDir(String dir) :2739, storeAndForwardDurability(SfDurability durability) :2761, storeAndForwardSyncIntervalMillis(long millis) :2814. maxBytes is now the only one that doesn't, and it reads as the total cap — precisely the confusion this rename exists to remove. Rename to maxSegmentBytes (4 uses: signature, guard :2782, .put(...) :2783, assignment :2785).

4. Javadoc is missing "WebSocket transport only." — in-diff, Sender.java:2771-2776.

/**
 * Maximum bytes per segment file before rotation. Defaults to
 * {@code DEFAULT_SEGMENT_BYTES}
 * (4 MiB). Smaller segments mean faster trim of acked data; larger
 * segments mean fewer rotations.
 */

storeAndForwardDir (:2732), storeAndForwardDurability (:2759) and storeAndForwardMaxTotalBytes (:2796) all carry that sentence; this method throws store_and_forward is only supported for WebSocket transport for HTTP/TCP/UDP, and this PR adds a test pinning exactly that. The wrap is also ragged ({@code DEFAULT_SEGMENT_BYTES} alone on :2774, (4 MiB). orphaned onto :2775) — worth re-flowing while you're in there, and naming the connect-string key sf_max_segment_bytes in the text.

5. New tests are misordered among themselves — in-diff, LineSenderBuilderWebSocketTest.java:638/649/666.
The insertion point is right, but after the shared prefix Reject, e < s. Required order (confirmed with LC_ALL=C sort -c):

testStoreAndForwardMaxSegmentBytes                                    (638, correct)
testStoreAndForwardMaxSegmentBytesRejectedForNonWebSocketTransports   (move up)
testStoreAndForwardMaxSegmentBytesRejectsNonPositiveValues

6. Existing rejection helper not reused — in-diff, LineSenderBuilderWebSocketTest.java:649-683.
~24 lines of inline try/fail/catch collapse to one call each against assertThrows(String expectedSubstring, Runnable action) at :1091. The helper asserts containment where the new tests assert equality — containment of the whole message is nearly as strong, and the consistency is worth it. (The assertThrowsAny(LineSenderBuilder, String...) overload at :1100 that testRetryTimeout_notSupportedForWebSocket uses is not applicable — it calls build(), and this setter throws eagerly during argument evaluation.)

Minor

7. testStoreAndForwardMaxSegmentBytes fails with a raw NPE on the exact regression it guards — LineSenderBuilderWebSocketTest.java:644. Mutation-verified: renaming the snapshot key produces NullPointerException: Cannot invoke "java.lang.Number.longValue()" because the return value of "java.util.Map.get(Object)" is null, reported as an Error with no mention of the offending key. WsSenderConfigHonoredTest.java:120-124 already has the right pattern — Assert.assertEquals(Long.valueOf(64 * 1024L), builder.wsConfigSnapshotForTest().get("sf_max_segment_bytes")) yields expected:<65536> but was:<null>.

8. Naming style. 89 of 115 tests in this file (77%) use _suffix, and it is near-universal for rejection tests specifically (testRetryTimeout_notSupportedForWebSocket, testMaxBackoff_notSupportedForWebSocket, testAutoFlushBytesNegative_fails). testStoreAndForwardMaxSegmentBytes_nonPositive_fails / _notSupportedForNonWebSocket would match the file and fix finding 5 for free.

9. The "which" → "that" commit is a regression, not a fix — LineSenderBuilderWebSocketTest.java:41. The clause is parenthesised and comments on an already-identified referent, so it is non-restrictive and takes which; that is restrictive. Revert the line.

10. Uncovered edge in the new rejection test. {0L, -1L} misses Long.MIN_VALUE, where LineSenderException.put(long) → Numbers.append(..., checkNaN=true) renders the literal string null, so production emits sf_max_segment_bytes must be positive: null. Pre-existing and shared by every sibling .put(value) message — not this PR's to fix, but worth knowing.

11. Connect-string non-WS message untested — Sender.java:3392, "sf_max_segment_bytes is only supported for WebSocket transport". Distinct string from the builder's "store_and_forward is only supported…" that new test 3 covers. The sibling pattern exists at SfFromConfigTest.java:335 (testSfSyncIntervalRejectsOnNonWebSocketTransport). Pre-existing gap carried through the rename.

12. PR body. The [!NOTE] about the red JDK 8 check is now stale — #71 is closed and all 14 checks on this PR are green. Remove it before merge. Also correct the release range per finding 2. Labels: only tandem; this repo's label set is thin enough that nothing else clearly fits, so that is fine.

Downgraded (false positives)

• "BLOCKING: parent-repo QwpIngressOracleFuzzTest breaks on submodule bump." Flagged by three agents. The breakage is real (8 ws:: strings at lines 215, 371, 562, 703, 810, 832, 986, 1006 would throw unknown configuration key: sf_max_bytes from ConfigView.java:83), but tandem PR refactor(qwp): rename sf_max_bytes to sf_max_segment_bytes in e2e fuzz test questdb#7413 is open with exactly QwpIngressOracleFuzzTest.java + the java-questdb-client pointer bump. Not a defect in this PR — just the stated merge order: this one first.
• "Sender.java:3394-3395 is dead code." True (fromConfig returns into fromConfigWebSocket at :3151 before the loop, so the protocol != PROTOCOL_WEBSOCKET guard at :3391 is always true), but pre-existing and shared by every ws-only key in that loop. The PR's edit to those lines is a harmless no-op.
• "Missing assertMemoryLeak() on the new tests." Correct to omit — ~70 sibling builder-only tests in the same class do, and these allocate no native memory.
• "Magic numbers need _ separators." 131072 and 4096 live inside String literals (connect-string text); 64 * 1024L is a 2- and a 4-digit literal. No violation.
• "Assert.assertSame(builder, …) is vacuous." It pins the fluent return this at :2786.
• "Comment lines over-long after the longer key name." CursorWebSocketSendLoop.java:186 (85 chars) and MmapSegment.java:222 (80) are ragged against their own paragraphs but under each file's existing maximum. Cosmetic.
• "ConfigSchema / field / method ordering broken by the rename." All three still sort correctly: sf_max_segment_bytes < sf_max_total_bytes (s < t); sfDurability → sfMaxSegmentBytes → sfMaxTotalBytes → sfSyncIntervalMillis → shouldDestroyPrivKey; storeAndForwardDir → Durability → MaxSegmentBytes → MaxTotalBytes → SyncIntervalMillis.

Summary

Approve with changes — nothing blocking. The rename is complete, mechanically inert, and CI is green on all 14 checks.

One missing thing is a test pinning the removed key as rejected (finding 1): this PR's entire user-visible effect is untested, and every existing guard is schema-derived and structurally blind to it. Findings 3 and 4 are cheap and belong in a rename PR specifically — the parameter still named maxBytes reintroduces the exact ambiguity the rename removes.

[mtopolnik]

[PR Coverage check]

😍 pass : 12 / 15 (80.00%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/client/Sender.java	11	14	78.57%
🔵	io/questdb/client/impl/ConfigSchema.java	1	1	100.00%