feat(python): add HttpConfig transport configuration - #3992
Conversation
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3992 +/- ##
============================================
+ Coverage 85.90% 85.92% +0.02%
Complexity 1431 1431
============================================
Files 1244 1246 +2
Lines 192435 192662 +227
Branches 158133 158293 +160
============================================
+ Hits 165307 165553 +246
+ Misses 23037 22989 -48
- Partials 4091 4120 +29
🚀 New features to boost your workflow:
|
ethanlin01x
left a comment
There was a problem hiding this comment.
I ran this locally and it works well. I have a few suggestions, mostly around tests and docs.
89fddbd to
2eaaada
Compare
|
@ethanlin01x Could you please check now. |
slbotbm
left a comment
There was a problem hiding this comment.
As was requested in the websocket pr: fold the http config example into existing examples as comments, and change the exposed API signature to IggyClient( ... | HttpConfig | ...).
2eaaada to
700ef37
Compare
|
/ready |
hubcio
left a comment
There was a problem hiding this comment.
my main worry is that HTTP now reads as a first-class transport without saying what it cannot do, and two of the gaps are silent.
A few findings land on code this PR doesn't touch, so I couldn't attach them inline:
core/sdk/src/http/http_client.rs:98,117,137,157,173,192,213- theaccess_tokenread guard is held across the whole.send().awaitin all seven verbs, including every retry backoff.IggyRwLockis write-preferring, so onelogin_userqueues behind in-flight requests and then blocks new readers. There's no request timeout either (:272is a barereqwest::Client::new()), so a hung server pins the guard with no bound.refresh_access_tokenat:312-320already does the clone-then-drop thing.foreign/python/README.md:137- lead-in still saysIggyClienttakes "either a server address or aTcpConfig", 34 lines above the new paragraph..github/actions/python-maturin/pre-merge/action.yml:167-${{ steps.iggy.outputs.address }}isn't an outputserver-startdeclares, so that line logs an empty address.tcp_addressis the one you want.foreign/python/src/client.rs:1117-consumer_group()is the one HTTP-hostile entry point with no HTTP note; join and leave both carry one.core/common/src/types/configuration/http_config/http_client_config.rs:47-55- separate from this PR, but worth an issue: the HTTP connection string requiresuser:pass@and then drops it, sofrom_connection_string("iggy+http://iggy:iggy@...")gives an unauthenticated client. tcp, quic and websocket all carryauto_loginthrough; http is the only one that doesn't. Same line hardcodeshttp://, soHttpConfigis the only Python route to an https endpoint.core/sdk/src/clients/client.rs:358- the doctest connection string has no@, so it can't parse. It'sno_run, so nothing catches it.
Assert heartbeat_interval in config tests, add a real HTTP send/poll round trip, rename TestClientConstruction to avoid colliding with test_client_config.py, and drop the inaccurate "stateless" framing around the missing AutoLogin.
Drop the IggyClient.http() classmethod in favor of extending IggyClient(...)'s existing TcpConfig union to also accept HttpConfig. Trims the HTTP section from the main README and folds the standalone http/ example into a commented alternative in the getting-started example, per review feedback.
The HTTP consumer-group note described a server-side fence that is never reached: Consumer::kind is #[serde(skip)], so a Consumer.Group(...) poll is served as an ordinary consumer named after the group and returns messages rather than an empty result. connect() is also kept in the HTTP guidance, since it is what starts the heartbeat that heartbeat_interval configures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The HTTP consumer-group documentation claimed that a Consumer.Group(...) poll degrades silently into an ordinary consumer named after the group. That is reachable only through a direct poll_messages carrying an explicit partition_id. IggyConsumer::new pins partition_id to None for every group member, and the default MessageClient::poll_messages_with_strategy_for rejects a consumer-group poll without a partition client-side, so consumer_group(...) fails with "Feature is unavailable" either at the join or, with auto_join_consumer_group off, at the first poll. Disabling auto-join is not a workaround. jwt was validated on trim() but stored untrimmed, so a token read from a file passed construction and then failed every call with InvalidHttpRequest once HeaderValue rejected the Authorization value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HttpConfig pushed every field through the builder and validated the URL last, so a doubly-invalid construction reported the field error while TcpConfig and QuicConfig reported the address error. Build from api_url first and assign the remaining fields afterwards, matching both siblings. Rename u32_arg to u32_param for the sibling helpers, name the QUIC port default, order the test helpers as TCP, QUIC, HTTP to match lib.rs and PyClientConfig, and state the missing AutoLogin and reconnection limitation on the class docstring so the stub is self-contained. Drop the getting-started note claiming main() logs args.tcp_server_address, which this branch made false by logging the config instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviewing the HTTP transport config turned up conventions the QUIC transport landed in apache#3991 does not yet follow, so this brings QUIC up to the same bar. Both reconnection policies and QuicConfig now document the OverflowError their i64 extraction raises, which only HttpConfig mentioned. QuicConfig's client_address is trimmed before it is parsed, so it tolerates surrounding whitespace like the server address and the HTTP API URL already do; the value is still stored verbatim rather than re-serialized, because QuicClient::create compares it against the literal default to pick an IPv6 bind address. Rejecting a malformed address forwarded a bare IggyError Display that named no argument, leaving a caller to guess which one it was. That is a uniform change across all three transports rather than a QUIC-only fix, so TcpConfig, QuicConfig and HttpConfig now all name the offending keyword, and the tests that asserted the old text follow. Two QUIC tests were passing on assertions that could not fail: client construction only checked the result was not None, which never pinned the transport, and the repr test covered neither the three string fields nor the duration boundary. The examples needed three manual edits to switch to QUIC where HTTP needed one uncomment, so QuicConfig is now imported and the return annotation widened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The test bound an unroutable client_address and expected the bind to fail, which only holds while net.ipv4.ip_nonlocal_bind is 0. Hosts running keepalived or HAProxy set it globally and a container can set it alone, and there the bind succeeds and the test asserts nothing. Binding a port that is already held fails regardless of the sysctl and still pins the transport, since client_address is QUIC-only. QuicReconnectionConfig open-coded the conversion its TcpReconnectionConfig sibling already delegates to u32_param, the jwt trim had no coverage, the README omitted the at-least-once warning the retries doc leads with, so a producer reading only the README would not know a lost response replays the body, and the CI comment claiming both ports come from server-start outputs was contradicted by the QUIC literal below it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
90ebe43 to
0661bf3
Compare
Which issue does this PR address?
Relates to #2835.
Rationale
Python's HTTP transport was only reachable through the untested from_connection_string() path, with no config object like TCP got in #3776.
Added
HttpConfig, accepted byIggyClient(...)alongside a plain address andTcpConfig, mirroring the TcpConfig pattern. An earlier revision exposed thisas an
IggyClient.http(...)classmethod; that was dropped in favour of wideningthe existing constructor union, per review.
Local Execution
cargo fmt,cargo clippy --all-targets -- -D warnings,ruff,pyrefly,markdownlintall cleanapache_iggy.pyiregenerated viastub_genand in synctest_tls.pyneeds Docker)
AI Usage
Claude Sonnet 5 and Claude Opus 5