Repository navigation
feat(go): parse iggy:// connection strings into TCP client options - #4497
choksi2212 wants to merge 10 commits into
Conversation
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
|
/skill team-review-slim |
|
Hi maintainers — this is my first PR to this repo, so the Pre-merge and PR Title workflows are waiting on maintainer approval ( |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4497 +/- ##
============================================
- Coverage 90.77% 90.73% -0.05%
Complexity 2989 2989
============================================
Files 1459 1461 +2
Lines 348165 348501 +336
Branches 292747 292747
============================================
+ Hits 316042 316196 +154
- Misses 25485 25654 +169
- Partials 6638 6651 +13
🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary: The new Go parser for iggy:// connection strings diverges from the shared grammar in its duration rules, drops the nodelay value although tcp.WithNoDelay exists, and can echo a credential from the unsupported-transport error. The remaining findings are small test and comment fixes.
Counts: critical 0, warning 4, nit 3, simplification 2
This review was generated by Claude Code 2.1.284 on deepseek-flash[1m]. Review the output before you act on it.
github-actions
Bot
added
S-waiting-on-author
Apply all nine review comments on apache#4497: - nodelay: the parser validated the spelling and dropped it, so ?nodelay=false still left Nagle disabled. The parsed value now reaches tcp.WithNoDelay, and the client default (Nagle off) is preserved when the option is absent. GetNoDelay exposes the setting, so a test can observe the option before a connection exists. - durations: reconnection_interval and heartbeat_interval are non-zero durations in the Rust SDK, so the zero spellings and zero-valued durations are now rejected for those two, while reestablish_after keeps accepting them, because Rust types it as a plain duration. - units: the duration table now matches the Node and C# SDKs, carrying hr, hrs, wk, wks and month and taking millis instead of milliseconds. - transport errors echo only an ASCII-letter transport name, so a malformed scheme with embedded credentials cannot leak them. - a bad port reports InvalidConnectionString like every other parse failure, so errors.Is covers the whole parse. - the IPv6 test is renamed for the bracketed host it passes. - the rejection table is attributed to the Node SDK, whose shapes it mirrors, not Rust. - the unused frameOffsetClient constant is deleted. - the frame offsets, fake server, reply builders and self-signed certificate helper move into one shared internal test-support package, internal/fakevsr, which both test binaries import; tcp_testing_test.go keeps only its TCP-specific fixtures. New tests pin each behavior change: nodelay landing on the client, the zero-spelling rejections per option, the extended duration units, and transport errors never echoing credentials. Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
|
Thanks for the detailed review — all nine comments are addressed in 57ce6bb, with a reply on each thread and a test pinning every behavior change:
Verified locally: One ask: the earlier BDD job failure on this PR was an infra flake, not the diff — the docker build got an HTTP 500 fetching |
|
The d38cad2 applies exactly
|
|
if you PR is ready for re-review use |
|
also please tone down the ai-generated replies and keep responses short and in your own words. this is starting to feel like i'm talking to an agent through you, and i'd like to hear your own reasoning about the changes. (if any) dont getg me wrong, AI tools are welcome here. our AI assistance policy still requires you to understand the code and take responsibility for what you submit. |
|
Fair enough. I lean on AI tooling while working through these, but the calls here are mine and I own every line of what I submit — keeping the replies short and in my own words from here. |
|
/ready All five points are in a1f46e4, each with a test: nodelay stays on when the query leaves it out, reconnection_interval and heartbeat_interval refuse anything under a microsecond like NonZeroIggyDuration, 1 s and µs parse the way humantime parses them, the nanosecond carry is gone, and the self-signed certificate helper lives in internal/fakevsr now. go test ./client/... -race is green, gofmt clean. |
github-actions
Bot
added
S-waiting-on-review
|
@choksi2212 please rebase this PR, a huge multi-leader related PR was merged and now there are conflicts. |
hubcio
added
S-waiting-on-author
Add ParseConnectionString to the Go SDK, so a caller can configure the
TCP client from a connection string the way the Rust, Node and C# SDKs
already do:
opts, err := client.ParseConnectionString(iggy://iggy:iggy@localhost:8090)
cli, err := client.NewIggyClient(client.WithTcp(opts...))
The parser accepts iggy:// and iggy+tcp:// with username:password or a
personal access token before the @, bracketed IPv6 hosts, and the
shared query-option grammar: tls, tls_domain and tls_ca_file are
applied through the existing WithTLS options, while the reconnect,
heartbeat and nodelay spellings are validated for cross-SDK
compatibility and then ignored, because the Go client takes those
from its own defaults. Other schemes, such as iggy+quic://, are
rejected.
Tests cover the valid shapes, both credential types, TLS (including a
full connect-and-login over TLS against a fake server), the malformed
strings each other SDK rejects, and that no error message leaks the
password or token.
Closes apache#4491
Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
Apply all nine review comments on apache#4497: - nodelay: the parser validated the spelling and dropped it, so ?nodelay=false still left Nagle disabled. The parsed value now reaches tcp.WithNoDelay, and the client default (Nagle off) is preserved when the option is absent. GetNoDelay exposes the setting, so a test can observe the option before a connection exists. - durations: reconnection_interval and heartbeat_interval are non-zero durations in the Rust SDK, so the zero spellings and zero-valued durations are now rejected for those two, while reestablish_after keeps accepting them, because Rust types it as a plain duration. - units: the duration table now matches the Node and C# SDKs, carrying hr, hrs, wk, wks and month and taking millis instead of milliseconds. - transport errors echo only an ASCII-letter transport name, so a malformed scheme with embedded credentials cannot leak them. - a bad port reports InvalidConnectionString like every other parse failure, so errors.Is covers the whole parse. - the IPv6 test is renamed for the bracketed host it passes. - the rejection table is attributed to the Node SDK, whose shapes it mirrors, not Rust. - the unused frameOffsetClient constant is deleted. - the frame offsets, fake server, reply builders and self-signed certificate helper move into one shared internal test-support package, internal/fakevsr, which both test binaries import; tcp_testing_test.go keeps only its TCP-specific fixtures. New tests pin each behavior change: nodelay landing on the client, the zero-spelling rejections per option, the extended duration units, and transport errors never echoing credentials. Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
CI's Format Check on the LF checkout flagged every file my two commits touched: the import blocks were not in gofmt order (the dot-import of internal/fakevsr sat first instead of sorted in, and crypto/tls sorted after crypto/x509 in the new parser test). This applies exactly 'gofmt -w' to the committed content of the fourteen files touched by 04be090 and 57ce6bb - import ordering only, no logic or line-ending changes in the blobs. Verified: gofmt -l foreign/go is empty on every staged blob; go build ./... and go vet ./... clean; go test -race -count=1 ./client/... green (client 3.8s, tcp 29.3s). The internal/vsr protocol-parity tests are untouched by this change and fail identically on pristine 57ce6bb on a CRLF Windows checkout (their Rust-source regexes anchor on comma-EOL), so they are a checkout artifact here, not a regression. Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
- keep TCP_NODELAY on when a query leaves nodelay out, matching the Go client default the other SDKs share - refuse reconnection_interval and heartbeat_interval below one microsecond, the resolution the Rust wire format carries - accept a space between an amount and its unit and the microsecond spelling, as humantime and the other SDK parsers do - drop a nanosecond carry the total already folded away - share the self-signed test certificate through internal/fakevsr Signed-off-by: Manas Choksi <choksi2212@users.noreply.github.com>
The transport tests moved to their multi-leader request helpers upstream, so the rebase keeps every transport test on those helpers and drops the sweep that pointed them at fakevsr. What stays shared, as the review asked, is the certificate helper: fakevsr.SelfSignedCert now backs selfSignedCert, and the fake roster reply gains the ReplicaID the multi-leader client expects. Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
a1f46e4 to
c44ae1c
Compare
|
Rebased onto master @ e9e5e10 and force-updated — thanks for the heads-up, the multi-leader rework touched exactly the helper territory this PR had refactored. How the conflicts resolved:
Verification on the new head:
The five review points are all still in: |
|
Thanks again for the multi-leader heads-up — the branch is rebased and |
|
@hubcio gently pinging this one for a re-review when you have a moment. All five points from your review are addressed in a1f46e4 (per-point summary in my /ready comment from Oct 13), and the rebased head 8ada46b has the full CI green (51 checks, including the Go build/test/e2e/lint suite). Happy to adjust anything else that does not look right. |
…n-string-4491 Resolved the single conflict (foreign/go/client/tcp/tcp_core.go) by taking upstream's restructured core and re-grafting GetNoDelay() after GetConnectionInfo, so the parsed nodelay option stays observable before the first connection. Verified on the merge result: full foreign/go test suite green (client, client/tcp incl. the connection-string tests, internal/vsr protocol parity), go vet clean, gofmt clean. Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
|
Rebased onto current The single conflict was in Verified on the merge result (
The five review points from the |
|
@choksi2212 when PR is ready for re-review put |
github-actions
Bot
added
S-waiting-on-review
|
/ready Rebased onto current master and verified locally on the merge result: go build/vet clean, gofmt clean, go test ./client/... -race green. |
github-actions
Bot
added
S-waiting-on-author
The parser validated heartbeat_interval but dropped it, so the client kept its five-second default no matter what the connection string asked for. The interval now reaches tcp.WithHeartbeatInterval after a checked int64 conversion, and a value wider than time.Duration fails the parse instead of wrapping. The tcp test helpers readRequest and clusterMetadataFrame now delegate to the shared fakevsr builders. Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
|
/ready heartbeat_interval now reaches tcp.WithHeartbeatInterval after a checked int64 conversion; a value wider than time.Duration fails the parse instead of wrapping. Pinned with a new test through a GetHeartbeatInterval accessor: default 5s without the option, 1s and 250ms applied, 1000y rejected. readRequest and clusterMetadataFrame in the tcp tests now delegate to the fakevsr builders, with a small adapter where the local request type differs. The full tcp suite passes with -race through the wrappers. |
github-actions
Bot
added
S-waiting-on-review
Resolve the connect-test imports against the apache#4571/apache#4625 upstream wave: keep the fakevsr delegation for the self-signed cert and add fmt/strings for the new silent sign-in tests. tcp_core.go and tcp_testing_test.go auto-merged. Verified on the merge result: go build ./... clean, go vet clean on client + fakevsr, client suite ok, full tcp suite green on rerun (the one first-run failure, TestDriverAfterSilentMetadataReplica- DiscoversThroughAnotherLink at 0.01s, also flakes on pristine upstream/master where a different tcp test failed its own full run). Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
|
Merged current Verified on the merge result:
One honest caveat about the tcp suite on my local box: across five full-package runs it intermittently failed three different tests ( |
…GetNoDelay lock Both fake-server helpers carried a done channel nothing ever read: the field, its allocation and its deferred close are gone from fakevsr.Server and from the tcp suite's fakeServer. GetNoDelay now reads the construction-frozen config directly, like the dial path does; GetHeartbeatInterval keeps its lock because a test rewrites config.heartbeatInterval after construction. Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
Which issue does this PR address?
Closes #4491
Rationale
The Rust, Node and C# SDKs all accept
iggy://connection strings; the Go SDK had no parser, so Go users built the TCP options by hand. This addsParseConnectionStringper the issue spec, returning the existing options fromtcp_core.goso the result drops straight intoclient.WithTcp.What changed?
foreign/go/client/connection_string.gowithfunc ParseConnectionString(connectionString string) ([]tcp.Option, error).iggy://andiggy+tcp://; otheriggy+<transport>schemes are rejected.username:passwordor a single personal access token before the@, wired intoWithAutoLoginviaNewUsernamePasswordCredentials/NewPersonalAccessTokenCredentials. No new credential or TLS types were added.host:portand bracketed IPv6 ([::1]:8090), validated withnet.SplitHostPortplus a u16 port parse.client.connection-string.ts):tls=true|false,tls_domain,tls_ca_fileare applied viaWithTLS/WithTLSDomain/WithTLSCAFile;reconnection_retries,reconnection_interval,reestablish_after,heartbeat_intervalandnodelayare validated for compatibility and then ignored, because the Go client takes reconnect, heartbeat and nodelay behavior from its own defaults. Unknown options and malformed values (tls=maybe,reconnection_retries=three,reconnection_interval=garbage) are rejected, so a connection string is either valid everywhere or nowhere.ierror.InvalidConnectionString{}andierror.InvalidNumberValue{}. No error message includes the connection string, so a rejected password or token never leaks into logs.How was it tested?
go test ./client/ -count=1— 11 test functions, all passing:iggy+tcp://with a personal access token, bracketed IPv6, port 0 (u16 rule shared with the other SDKs).NewIggyClient(WithTcp(opts...))connects and logs in against a fake single-node VSR server, asserting the register frame arrives before the auth-gated roster read. The same flow runs over TLS withtls=true&tls_domain=localhost&tls_ca_file=...against a TLS-only fake server, proving the option survives into the client config.iggy+quic://,iggy+ws://and non-iggy schemes; also fractional/multi-unit durations (0.5s,1.005s,1h 30m,1h30m,5sec) are accepted while5andgarbageare not, andreconnection_retriesacceptsunlimitedand u32 values but rejects4294967296.connection_string.goremoved, the test file fails to compile (undefined: ParseConnectionString), confirming the feature is new.go test ./... -count=1 -racegreen across the module (client, tcp, errors, codec, command, hash, util, tests);go vet ./...clean;gofmtclean;go mod tidyproduces no diff. The only failure in a plaingo test ./...on this Windows checkout isinternal/vsrTestProtocolParity_*, which fails identically on pristine master here because the CRLF checkout breaks the Rust-source regexes — unrelated to this change, and CI (Linux) is unaffected.Documentation
The package-level doc comment on
ParseConnectionStringdocuments the accepted shapes and the cross-SDK grammar, including which options are ignored and why.