yummyPancake2607 commented on PR #4065:
URL: https://github.com/apache/iggy/pull/4065#issuecomment-5551209737
@slbotbm
## get_client: OverflowError vs RuntimeError for out-of-range client_id
You're right — `client_id: u32` is converted by pyo3 before the coroutine
even exists, so `get_client(-1)` raises `OverflowError` synchronously, not
`RuntimeError` from within the request.
Fixed in `foreign/python/src/client.rs`:
- The `Args:` section now says the conversion happens before the awaitable
exists and names `OverflowError` as the result for an out-of-range value.
- Added `OverflowError` as its own line in `Raises:`, ahead of the
`RuntimeError` line.
Added
`tests/test_client_info.py::TestGetClient::test_get_client_out_of_range_id_raises_overflow_error`,
parametrized over `[-1, 2**32]` (`ids=["negative", "above-u32"]`), following
`test_client_config.py:130`'s `out_of_range` pattern rather than
`test_user.py`'s (that one asserts `TypeError` because `get_user` takes the
`str | int` union `PyIdentifier`, not a plain `u32`). Verified it fails
before the fix and passes after, against a live server.
## get_me: unimplemented over HTTP
Confirmed — `core/sdk/src/http/system.rs:46` is
`async fn get_me(&self) -> ... { Err(IggyError::FeatureUnavailable) }`
unconditionally, while `get_client`/`get_clients` do issue real requests
over HTTP (`GET /clients/{id}` and `GET /clients`).
Added a line to `get_me`'s docstring stating it's unimplemented over HTTP
and always raises, unlike `get_client`/`get_clients`, and updated `Raises:`
to name `Feature is unavailable` as the unconditional HTTP outcome.
## get_clients completeness assumption
Confirmed against `core/shard/src/lib.rs:1840` (`list_all_clients`): it's a
scatter-gather across shards bounded by `LIST_CLIENTS_GATHER_TIMEOUT` (3s,
`core/shard/src/lib.rs:312`), and a shard that misses the deadline is
dropped from the result — "callers should treat the result as
best-effort-complete" per that function's own doc comment.
Added a comment explaining why completeness holds anyway at each place the
tests rely on it:
- `test_get_clients_contains_this_client` — the membership check now uses
`next((c for c in clients if ...), None)` instead of a list-length
assertion, with the comment explaining the test server is unloaded and
single-node so every shard replies well within the 3s budget.
- `test_user_with_read_servers_can_list_clients` — same assumption, shorter
comment pointing back to the first one rather than repeating it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
