feat: add multi-url failover to Python bindings#126
Conversation
Expose multi-url failover in Python via config strings with multiple `addr` keys and a new `addresses` keyword argument on Sender.__init__ for programmatic use. Update type stubs and add 14 tests covering basic usage, failover behavior, error handling, and IPv6 parsing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix resource leak in test_multi_url_ipv6_from_conf (add finally/close) - Add error code assertion to test_addresses_tcp_rejected - Add tests: tcps rejection, all-servers-fail, failover body content, list entries in addresses param - Pre-load sufficient 500 responses for all-servers-fail test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds multi-address (multi-URL) failover support: Sender accepts an Changes
Sequence DiagramsequenceDiagram
participant Client
participant Sender
participant ConfParser
participant CAPI
Client->>Sender: init(..., addresses=[('host1', 8080), ('host2', 8080)])
Sender->>CAPI: line_sender_opts_new()
alt Addresses provided programmatically
loop for each (host,port)
Sender->>CAPI: line_sender_opts_address(host, port)
CAPI-->>Sender: status / err
end
else Addresses from config
Sender->>ConfParser: parse_conf_str(conf_string)
ConfParser-->>Sender: params (includes _addrs)
loop for each addr in _addrs
Sender->>CAPI: line_sender_opts_address(host, port)
CAPI-->>Sender: status / err
end
end
CAPI-->>Sender: configured opts or error
Sender-->>Client: ready or raise error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use your project's `ruff` configuration to improve the quality of Python code reviews.Add a Ruff configuration file to your project to customize how CodeRabbit runs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/questdb/ingress.pyi (1)
834-861:⚠️ Potential issue | 🟡 MinorAlign the stub with the accepted
addressesentry shape.
src/questdb/ingress.pyxaccepts both tuple and list entries inaddresses, andtest/test.pynow asserts thataddresses=[['127.0.0.1', port]]works. This stub only allowsList[Tuple[...]], so type-checked callers will get a false error for a supported input. Either widen the annotation here or stop accepting list entries at runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/questdb/ingress.pyi` around lines 834 - 861, The stub's addresses parameter only allows List[Tuple[..., ...]] but runtime accepts lists as well; update the signature in src/questdb/ingress.pyi to widen addresses to accept both tuples and lists (e.g. change addresses: Optional[List[Tuple[str, Union[int, str]]]] to addresses: Optional[List[Sequence[Union[str, int, str]]]] or addresses: Optional[List[Sequence[Union[str, int]]]]), and add Sequence to the typing imports so type-checkers won't flag callers using addresses=[['127.0.0.1', port]].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/system_test.py`:
- Around line 331-334: The test_sending_just_timestamps test is currently a
no-op because sender.row(..., at=TimestampNanos.now()) doesn't write unless at
least one column/symbol is present; update the test to either assert the no-op
explicitly (e.g., check that no rows were created) or send a real field/symbol
and then verify persistence. Modify the call to sender.row in
test_sending_just_timestamps to include a concrete column or symbol (e.g., add a
field name/value) and after sender.flush() query the table to assert the row
exists and has the expected timestamp, or alternatively add an explicit
assertion that no rows were written to confirm the no-op behavior; refer to
sender.row, TimestampNanos.now(), and the ingestion behavior in
src/questdb/ingress.pyx when making the change.
---
Outside diff comments:
In `@src/questdb/ingress.pyi`:
- Around line 834-861: The stub's addresses parameter only allows
List[Tuple[..., ...]] but runtime accepts lists as well; update the signature in
src/questdb/ingress.pyi to widen addresses to accept both tuples and lists (e.g.
change addresses: Optional[List[Tuple[str, Union[int, str]]]] to addresses:
Optional[List[Sequence[Union[str, int, str]]]] or addresses:
Optional[List[Sequence[Union[str, int]]]]), and add Sequence to the typing
imports so type-checkers won't flag callers using addresses=[['127.0.0.1',
port]].
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc03a00f-2552-4b8b-a53d-d8bf8a59e199
📒 Files selected for processing (6)
c-questdb-clientsrc/questdb/ingress.pyisrc/questdb/ingress.pyxsrc/questdb/line_sender.pxdtest/system_test.pytest/test.py
| def test_sending_just_timestamps(self): | ||
| with qi.Sender('http', 'localhost', self.qdb_plain.http_server_port) as sender: | ||
| sender.row(table_name="just_timestamp_test", at=TimestampNanos.now()) | ||
| sender.flush() |
There was a problem hiding this comment.
This test is currently a no-op.
sender.row(..., at=TimestampNanos.now()) does not write anything unless at least one symbol or column is present, so this path just rewinds the buffer in src/questdb/ingress.pyx. With no assertion afterwards, the test stays green even if timestamp-only writes are unsupported or broken. Please either assert the no-op explicitly or add a real field and verify the row was persisted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/system_test.py` around lines 331 - 334, The test_sending_just_timestamps
test is currently a no-op because sender.row(..., at=TimestampNanos.now())
doesn't write unless at least one column/symbol is present; update the test to
either assert the no-op explicitly (e.g., check that no rows were created) or
send a real field/symbol and then verify persistence. Modify the call to
sender.row in test_sending_just_timestamps to include a concrete column or
symbol (e.g., add a field name/value) and after sender.flush() query the table
to assert the row exists and has the expected timestamp, or alternatively add an
explicit assertion that no rows were written to confirm the no-op behavior;
refer to sender.row, TimestampNanos.now(), and the ingestion behavior in
src/questdb/ingress.pyx when making the change.
There was a problem hiding this comment.
This is pre-existing code, not introduced by this PR — out of scope.
There was a problem hiding this comment.
@nwoolmer, the diff actually shows this test was added in this PR — all four lines carry + markers, indicating they are new additions. The AI-generated summary also confirms it: "Added new test method test_sending_just_timestamps to send a row with only a timestamp."
Given that, the concern stands: as written the test is effectively a no-op (no column/symbol → nothing is written → no assertion). Would you like to either:
- Assert the no-op explicitly — e.g., verify no rows were persisted, or
- Add a real field and then query the table to confirm the row exists (matching the pattern of other tests in the file)?
Accept both tuples and lists in type annotations, matching runtime behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/questdb/ingress.pyi (1)
39-39: UnusedTupleimport.
Tupleis imported but not used anywhere in this file. Consider removing it to keep imports clean.🧹 Suggested fix
-from typing import Any, Dict, List, Optional, Sequence, Tuple, Union +from typing import Any, Dict, List, Optional, Sequence, Union🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/questdb/ingress.pyi` at line 39, Remove the unused Tuple symbol from the typing import list in the module (the line that currently imports "Any, Dict, List, Optional, Sequence, Tuple, Union"); update the import to only include the used names (e.g., "Any, Dict, List, Optional, Sequence, Union") so the unused Tuple is no longer imported and the file passes linting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/questdb/ingress.pyi`:
- Line 39: Remove the unused Tuple symbol from the typing import list in the
module (the line that currently imports "Any, Dict, List, Optional, Sequence,
Tuple, Union"); update the import to only include the used names (e.g., "Any,
Dict, List, Optional, Sequence, Union") so the unused Tuple is no longer
imported and the file passes linting.
|
Type stub fix for |
- Host must be str (reject None, int, etc. with TypeError) - Error messages include entry index: "addresses[0] must be..." - Type stub narrowed to Sequence[Tuple[str, Union[str, int]]] - 4 new tests for bad host types and wrong-length tuples Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
addrkeysaddresseskeyword argument toSender.__init__for programmatic use:addresses=[('host2', 9000), ('host3', 9001)].pyitype stubs and.pxdCython declarationsDepends on
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests