refactor(qwp): consolidate the QWP connect-string configuration keys - #66
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughQWP configuration now uses segment-specific store-and-forward sizing, removes in-flight-window and Gorilla configuration options, tightens key-scope parsing, adds connect-timeout handling, fixes encoder invariants, and removes standalone query authorization support. Tests and documentation reflect these contracts. ChangesQWP configuration contracts
QWP sender runtime
QWP wire and query contracts
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Drop the `auth` connect-string key and the WithQwpQueryAuth option from the QWP query client. Both supplied a pre-formed HTTP Authorization header value verbatim; credentials must now be given as the structured `username`/`password` or `token` keys, from which the client synthesizes the header downstream (Basic or Bearer). The mutual-exclusivity check covers username/password versus token; the `authorization` config field is gone. `auth=` is now rejected as an unsupported option, and the ingress LineSender never accepted the key, so it now fails on both directions. This mirrors the Java client change of the same name. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop the `in_flight_window` config-string key and the WithInFlightWindow option from the QWP sender. The value never reached the sender -- the cursor architecture governs backpressure via the segment ring and append deadline -- so it was a dead no-op. The key now falls through to the generic parser path and is rejected as `unsupported option "in_flight_window"`. Remove the now-dead config field, default constant, validation, and the constructor plumbing that carried the value, and update the docs that described it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop the WithGorilla option and the gorilla=on|off connection-string key. The QWP encoder now always sets FLAG_GORILLA and unconditionally applies Gorilla delta-of-delta timestamp encoding on ingress, keeping the per-column fallback to uncompressed when DoDs overflow int32. The egress query decoder is unchanged; it still reads the server's FLAG_GORILLA bit and handles both encoded and raw timestamps. Remove the gorilla config-parsing, encoder-disabled, and audit tests, and the CLAUDE.md note that documented the parameter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
path, client_id, server_info_timeout_ms, and replay_exec are egress-only QWP keys that the query client (parseQwpQueryConf) actively parses. A single ws:: / wss:: connect string must drive both the Sender and the QwpQueryClient, so the ingress LineSender has to silently accept the egress-only keys rather than reject them. These four were absent from egressOnlyKeys, so the ingress default branch rejected them with "unsupported option". Add them to the map and extend TestConfIngestSilentlyAcceptsEgressKeys. request_timeout, retry_timeout, request_min_throughput, and protocol_version are deliberately left out: sanitizeQwpConf rejects them on the QWP ingress as HTTP-only keys (TestConfQwpRejectsRetryTimeout), so the query client rejecting them too is consistent, not a portability gap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On QWP (ws:: / wss::) the four legacy ILP HTTP keys -- request_timeout, retry_timeout, request_min_throughput, and protocol_version -- are not part of the connect-string vocabulary (connect-string.md Key index). sanitizeQwpConf already rejected the first three and protocol_version when set to a concrete version. protocol_version=auto slipped through, because "auto" leaves the field unset and the sanitizer only inspects the field. The parser now rejects protocol_version outright on QWP, so even "auto" is caught, matching the egress QwpQueryClient and the Java, Rust, and .NET clients. New tests lock all four keys as rejected on both the ingress and egress QWP paths, and confirm they remain valid on the http:: schema. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A single ws:: / wss:: connect string is shared by the QWP ingress sender and the egress query client. Each parser now accepts the full QWP key vocabulary and silently ignores the keys meant for the other side, so one connect string configures both. The ingress parser accepts the egress-only keys; the egress parser ignores the ingress-only keys, including the user / pass auth aliases, transaction, and connection_listener_inbox_capacity. Both parsers reject keys outside the QWP vocabulary: - Legacy ILP keys (protocol_version including =auto, request_timeout, request_min_throughput, retry_timeout) are rejected on QWP while http/tcp still accept them. - ECDSA token_x / token_y are rejected on the QWP path, restoring ingress/egress symmetry. - The UDP-only keys max_datagram_size and multicast_ttl are rejected on the QWP path; this client has no UDP transport. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The connect-string key caps a single SF segment, and the new name says so; it also pairs cleanly with sf_max_total_bytes. The public option func follows: WithSfMaxBytes is now WithSfMaxSegmentBytes. Internal identifiers, error messages, tests, and docs rename in step so the old spelling is gone entirely. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
07c60b2 to
7b87d91
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 120-125: Update the QWP connect-string documentation section in
CLAUDE.md to list transaction, connection_listener_inbox_capacity, and user/pass
as Java-parity keys. Describe each key’s QWP scope and its no-op or alias
behavior, and retain the requirement that these compatibility keys must not be
removed without a corresponding Java change.
In `@conf_parse.go`:
- Around line 606-622: In the max_datagram_size/multicast_ttl handling within
the configuration parser, validate v with strconv.Atoi before checking
senderConf.senderType against qwpSenderType, so malformed values always produce
the invalid-integer error before any scope rejection. Extend
TestConfQwpRejectsUdpKeys with malformed ws:: cases covering these UDP keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea773527-f075-4156-8d9b-f89a7b982bc7
📒 Files selected for processing (21)
CLAUDE.mdREADME.mdconf_audit_test.goconf_parse.goconf_test.goexamples/qwp/sf/main.goqwp_constants.goqwp_encoder.goqwp_encoder_test.goqwp_error_resilience_test.goqwp_ingress_oracle_fuzz_test.goqwp_integration_test.goqwp_query_client.goqwp_query_client_test.goqwp_query_conf.goqwp_sender.goqwp_sender_cursor.goqwp_sender_test.goqwp_sf_conf_test.goqwp_sf_fallocate_unix_other.gosender.go
💤 Files with no reviewable changes (2)
- qwp_constants.go
- qwp_encoder_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- examples/qwp/sf/main.go
- qwp_sf_fallocate_unix_other.go
- README.md
- qwp_error_resilience_test.go
- qwp_ingress_oracle_fuzz_test.go
- qwp_sf_conf_test.go
mtopolnik
changed the title
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
conf_parse.go (1)
622-638: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUDP-key validation order still rejects malformed values with the wrong error on QWP.
This is the exact ordering flagged in a prior review:
max_datagram_size=abcon aws::string returns the QWP-scope error instead of the "not a valid int" error, because the scope check runs beforestrconv.Atoi. The code here is unchanged from the previously-flagged version — the fix (validate shape first, then reject scope) was not applied.🐛 Proposed fix
case "max_datagram_size", "multicast_ttl": + if _, err := strconv.Atoi(v); err != nil { + return nil, NewInvalidConfigStrError( + "invalid %s value, %q is not a valid int", k, v) + } if senderConf.senderType == qwpSenderType { return nil, NewInvalidConfigStrError( "%s is not supported for QWP; it applies to the UDP transport only", k) } - if _, err := strconv.Atoi(v); err != nil { - return nil, NewInvalidConfigStrError( - "invalid %s value, %q is not a valid int", k, v) - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@conf_parse.go` around lines 622 - 638, In the max_datagram_size/multicast_ttl handling, perform the strconv.Atoi shape validation before checking senderConf.senderType against qwpSenderType, so malformed values return the “not a valid int” error even for QWP; retain the existing QWP unsupported-key error for valid values.
🧹 Nitpick comments (1)
conf_parse.go (1)
168-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the
"user"/"pass"arms from the canonical-key loop.
parseConfigStrcanonicalizesuser→usernameandpass→passwordbefore populatingdata.KeyValuePairs, so these aliases can never appear askinconfFromStr. The alias behavior is still handled bycanonicalConfigKey; keeping these arms is only misleading dead code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@conf_parse.go` around lines 168 - 183, Remove the "user" and "pass" cases from the key switch in confFromStr, leaving only the canonical "username" and "password" handlers. Preserve alias support through canonicalConfigKey, which normalizes these keys before confFromStr processes them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@conf_parse.go`:
- Around line 622-638: In the max_datagram_size/multicast_ttl handling, perform
the strconv.Atoi shape validation before checking senderConf.senderType against
qwpSenderType, so malformed values return the “not a valid int” error even for
QWP; retain the existing QWP unsupported-key error for valid values.
---
Nitpick comments:
In `@conf_parse.go`:
- Around line 168-183: Remove the "user" and "pass" cases from the key switch in
confFromStr, leaving only the canonical "username" and "password" handlers.
Preserve alias support through canonicalConfigKey, which normalizes these keys
before confFromStr processes them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0da49f13-a3ae-4209-89fc-d69c311b95d6
📒 Files selected for processing (7)
CLAUDE.mdconf_audit_test.goconf_parse.goqwp_query_conf.goqwp_query_failover.goqwp_transport.gosender.go
🚧 Files skipped from review as they are similar to previous changes (1)
- sender.go
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@qwp_query_client.go`:
- Around line 447-456: Update WithQwpQueryConnectTimeout to reject negative
durations before calling d.Milliseconds(), ensuring sub-millisecond negative
values cannot become zero and bypass validation. Preserve the existing
conversion and zero-duration behavior for non-negative durations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c33890e-5709-4c78-b2cd-b9419ef5c805
📒 Files selected for processing (6)
conf_test.goqwp_error_resilience_test.goqwp_ingress_oracle_fuzz_test.goqwp_query_client.goqwp_query_client_test.gosender.go
🚧 Files skipped from review as they are similar to previous changes (5)
- qwp_error_resilience_test.go
- qwp_query_client_test.go
- qwp_ingress_oracle_fuzz_test.go
- conf_test.go
- sender.go
WithConnectTimeout and WithQwpQueryConnectTimeout convert a time.Duration to integer milliseconds by truncation. A negative duration whose magnitude is below one millisecond truncated to 0, which reads as the valid zero (OS-default) case, so the negative value silently bypassed the "negative durations are rejected" check in sanitizeQwpConf / validate(). Both options now map any negative duration to -1 ms, which the existing construction-time check rejects, while non-negative durations keep the prior conversion and zero (OS-default) behavior. Add white-box regression tests on both the ingest and query sides covering sub-millisecond negatives, a whole-millisecond negative, and the zero case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review: PR #66Reviewing at level 3 (full mission-critical pass), scoped to in-diff issues. This is a QWP-config refactor: rename Verification performed: CriticalNone. No correctness, panic, concurrency, leak, or wire-format regression found. The encoder change removes a branch (never adds a hot-path allocation); the default wire output is unchanged since Gorilla was already the default-on path. Moderate1. The
The functional risk is essentially nil (accepting the standard Minor1. Dead 2. 3. Two spellings of the same ms conversion in sibling functions. Ingress 4. Connect-timeout test gaps. No test asserts the opts→dialer wiring at Summary
One process note: the two open P-comments from the earlier commits ( |
parseConfigStr canonicalizes the `user` / `pass` aliases to `username` / `password` before confFromStr iterates the key/value map, so the `user` and `pass` case labels never matched; confFromStr only ever sees the canonical keys. Drop the unreachable labels and reword the comments to state that each canonical case handles both spellings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Brings in the mainline QWP work (#66 connect-string key consolidation, #67 connection recycle) on top of the qwip_go feature branch (delta symbol dictionary, NACK policy v2, the QuestDB facade, durable-ack). Conflict resolution notes: - NACK policy: kept v2 (PolicyRetriable/Terminal + poison-frame detector); main's PolicyHalt/DropAndContinue is superseded and undefined post-merge. - Config keys: adopted main's #66 policy per request — sf_max_segment_bytes rename, canonicalConfigKey aliases, duplicate-key rejection, transaction / UDP-key handling, and the strict QWP key vocabulary (gorilla, in_flight_window, token_x/token_y, protocol_version, egress auth are rejected/removed). Kept ours' orthogonal features (facade pool keys, durable-ack, connection-listener storage, delta symbol dictionary). - connect_timeout: combined both branches' edge-case fixes (reject negatives, floor sub-millisecond positives to 1ms). - README: dropped the duplicated legacy QWP section in favour of the reorganized facade-first structure. go build ./..., go vet ./..., and the unit suite all pass.
3 participants
Consolidates the QWP (
ws::/wss::) connect-string configuration vocabulary. A single connect string has to drive both the ingressLineSenderand the egressQwpQueryClient, so this PR settles which keys each side accepts, drops keys that no longer carry meaning, tightens per-transport validation, and renames one key for clarity. Almost every change is confined to the QWP transport; the sole cross-transport change is theuser/passcredential aliasing, which resolves in the shared connect-string tokenizer and so also affects the legacy ILPhttp(s)::/tcp(s)::schemas (detailed under One connect string for ingress and egress below).Removed QWP options
auth/WithQwpQueryAuth— the query client no longer accepts a pre-formedAuthorizationheader value. Credentials are given as the structuredusername/passwordortokenkeys, from which the client synthesizes the Basic or Bearer header downstream. Mirrors the Java client change of the same name.in_flight_window/WithInFlightWindow— a dead no-op. The value never reached the sender: the cursor architecture governs backpressure through the segment ring and the append deadline, andFlushnever waits for the ACK. The key now falls through to the generic parser and is rejected as an unsupported option.gorilla/WithGorilla— the encoder now always setsFLAG_GORILLAand unconditionally applies Gorilla delta-of-delta timestamp encoding on ingress, keeping the per-column fallback to uncompressed when the delta-of-deltas overflow int32. The egress decoder is unchanged — it still reads the server'sFLAG_GORILLAbit and handles both encoded and raw timestamps.One connect string for ingress and egress
path,client_id,server_info_timeout_ms,replay_exec) and the Java-parity ingress keys the doc Key index omits (transaction,connection_listener_inbox_capacity) as validated no-ops or equivalents.Either side parses the shared string without rejecting the other side's keys.
user/passare shared aliases, not QWP-scoped. They canonicalize tousername/passwordin the shared connect-string tokenizer (canonicalConfigKey), ahead of any per-schema handling, so they resolve on every transport, not just QWP. On QWP andhttp(s)::they map to the Basic-auth user/password; ontcp(s)::,usermaps to the key id exactly asusernamealways has (passstays rejected there, aspasswordalways was). This is a deliberate widening: those spellings were previously rejected as unsupported options on the legacy ILP transports. No existing valid config changes meaning — the mapping matches howusername/passwordalready behaved on each transport.Non-QWP keys rejected on the QWP path
protocol_version(including=auto, which previously slipped through because it leaves the field unset),request_timeout,request_min_throughput,retry_timeout— are rejected on QWP whilehttp::/tcp::still accept them. The protocol version is negotiated at the WebSocket upgrade.token_x/token_yare rejected on QWP, restoring ingress/egress symmetry (they remain ignored on the legacy ILP transports, which don't need the public key).max_datagram_size/multicast_ttlare rejected on QWP; this client has no UDP transport.This matches the egress
QwpQueryClientand the Java, Rust, and .NET clients.Rename:
sf_max_bytes→sf_max_segment_bytesThe key caps a single store-and-forward segment, and the new name says so; it also pairs cleanly with
sf_max_total_bytes. The public option follows:WithSfMaxBytesis nowWithSfMaxSegmentBytes. Internal identifiers, error messages, tests, the README, and the SF example rename in step, so the repo greps zero for the old spelling in any case variant.Cross-repo parity
Several changes track the Java client (
Sender.java): thesf_max_bytesrename (matching questdb/java-questdb-client#70), theauthremoval, and the shared ingress/egress key vocabulary. A connect string that works on the Java QWP client is meant to work here.API impact
This is almost entirely confined to the QWP transport, which is still being stabilized ahead of its first stable release — the full transport (store-and-forward cursor engine, query client, typed errors, failover) landed in #62, after v4.2.0. The lone exception is the shared-tokenizer
user/passaliasing described above, which now also parses onhttp(s)::/tcp(s)::(spellings previously rejected as unsupported options); it only widens what parses and leaves every existing valid config unchanged.sf_max_bytes/WithSfMaxBytesandWithQwpQueryAuthare part of that unreleased surface.WithGorillaandWithInFlightWindowshipped with the QWP ingress in v4.2.0, so their removal is a breaking change (flagged under Breaking Changes below).Verification
go build ./...andgo vet ./...are green locally; the Docker integration suite is CI's to confirm.🤖 Generated with Claude Code
Summary by CodeRabbit
sf_max_bytes→sf_max_segment_bytes(andWithSfMaxBytes→WithSfMaxSegmentBytes).WithInFlightWindowandWithGorilla; QWP no longer acceptsin_flight_window/gorillaconnect-string knobs.WithQwpQueryAuth; query connect strings no longer accept the legacyauthkey.connect_timeoutfor QWP connect strings.WithQwpQueryConnectTimeoutfor QWP query clients.sf_max_segment_bytes.