Skip to content

feat(python): add HttpConfig transport configuration - #3992

Merged
spetz merged 12 commits into
apache:masterfrom
saie-ch:python-http-config
Sep 7, 2026
Merged

feat(python): add HttpConfig transport configuration#3992
spetz merged 12 commits into
apache:masterfrom
saie-ch:python-http-config

Conversation

@saie-ch

@saie-ch saie-ch commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

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 by IggyClient(...) alongside a plain address and
TcpConfig, mirroring the TcpConfig pattern. An earlier revision exposed this
as an IggyClient.http(...) classmethod; that was dropped in favour of widening
the existing constructor union, per review.

Local Execution

  • cargo fmt, cargo clippy --all-targets -- -D warnings, ruff, pyrefly,
    markdownlint all clean
  • apache_iggy.pyi regenerated via stub_gen and in sync
  • Full Python suite passes against a local server (524 passed; test_tls.py
    needs Docker)
  • Both getting-started examples run, including with the HTTP config swap applied
  • Pre-commit hooks ran

AI Usage

Claude Sonnet 5 and Claude Opus 5

@github-actions

Copy link
Copy Markdown

Thanks for the PR. It is labeled S-waiting-on-review and queued for review.

Slash commands (own line, regular comment) move it around the queue:

  • /ready - back to S-waiting-on-review after addressing feedback
  • /author - flip to S-waiting-on-author while you finish changes
  • /request-review @user-or-team - request a reviewer

See CONTRIBUTING.md for details.

@github-actions github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label Aug 29, 2026
@codecov

codecov Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.82353% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.92%. Comparing base (b87b6a6) to head (63dba7b).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
foreign/python/src/config.rs 98.73% 1 Missing ⚠️
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     
Components Coverage Δ
Rust Core 86.84% <75.82%> (+0.02%) ⬆️
Java SDK 67.52% <ø> (ø)
C# SDK 76.99% <ø> (+0.01%) ⬆️
Python SDK 90.73% <98.82%> (+0.18%) ⬆️
PHP SDK 85.65% <ø> (ø)
Node SDK 96.26% <ø> (+<0.01%) ⬆️
Go SDK 69.43% <ø> (+0.03%) ⬆️
Files with missing lines Coverage Δ
foreign/python/src/client.rs 99.87% <100.00%> (+<0.01%) ⬆️
foreign/python/src/lib.rs 100.00% <100.00%> (ø)
foreign/python/src/config.rs 97.84% <98.73%> (+0.08%) ⬆️

... and 39 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ethanlin01x ethanlin01x left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran this locally and it works well. I have a few suggestions, mostly around tests and docs.

Comment thread foreign/python/tests/test_http_config.py
Comment thread foreign/python/tests/test_http_config.py
Comment thread examples/python/http/producer.py Outdated
Comment thread foreign/python/tests/test_http_config.py
Comment thread foreign/python/tests/test_http_config.py Outdated
@saie-ch
saie-ch force-pushed the python-http-config branch from 89fddbd to 2eaaada Compare August 31, 2026 04:21
@saie-ch

saie-ch commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

@ethanlin01x Could you please check now.

@slbotbm slbotbm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | ...).

@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Aug 31, 2026
@saie-ch

saie-ch commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

/ready

@github-actions github-actions Bot added S-waiting-on-review PR is waiting on a reviewer and removed S-waiting-on-author PR is waiting on author response labels Sep 5, 2026

@hubcio hubcio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 - the access_token read guard is held across the whole .send().await in all seven verbs, including every retry backoff. IggyRwLock is write-preferring, so one login_user queues behind in-flight requests and then blocks new readers. There's no request timeout either (:272 is a bare reqwest::Client::new()), so a hung server pins the guard with no bound. refresh_access_token at :312-320 already does the clone-then-drop thing.
  • foreign/python/README.md:137 - lead-in still says IggyClient takes "either a server address or a TcpConfig", 34 lines above the new paragraph.
  • .github/actions/python-maturin/pre-merge/action.yml:167 - ${{ steps.iggy.outputs.address }} isn't an output server-start declares, so that line logs an empty address. tcp_address is 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 requires user:pass@ and then drops it, so from_connection_string("iggy+http://iggy:iggy@...") gives an unauthenticated client. tcp, quic and websocket all carry auto_login through; http is the only one that doesn't. Same line hardcodes http://, so HttpConfig is 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's no_run, so nothing catches it.

Comment thread foreign/python/src/config.rs Outdated
Comment thread foreign/python/src/config.rs Outdated
Comment thread foreign/python/README.md Outdated
Comment thread foreign/python/src/config.rs
Comment thread examples/python/getting-started/producer.py Outdated
Comment thread foreign/python/src/config.rs Outdated
Comment thread foreign/python/src/client.rs
Comment thread foreign/python/tests/utils.py
Comment thread foreign/python/src/config.rs Outdated
Comment thread foreign/python/tests/test_http_config.py
@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Sep 5, 2026
saie-ch and others added 10 commits September 7, 2026 13:28
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>
@hubcio
hubcio force-pushed the python-http-config branch from 90ebe43 to 0661bf3 Compare September 7, 2026 14:31
Comment thread examples/python/http/producer.py Outdated
Comment thread foreign/python/src/config.rs
Comment thread foreign/python/README.md Outdated
@spetz
spetz dismissed slbotbm’s stale review September 7, 2026 17:30

Already accepted

@spetz
spetz merged commit bd1a873 into apache:master Sep 7, 2026
99 checks passed
@github-actions github-actions Bot removed the S-waiting-on-author PR is waiting on author response label Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants