justinmclean commented on PR #4018: URL: https://github.com/apache/iggy/pull/4018#issuecomment-5503337640
Nice addition, follows the existing `user.rs` wrapper pattern and the stub is regenerated. Nothing blocking; a few things worth a look. - `src/stats.rs:30` — `CacheMetricsKey` has no `#[new]`, so Python can't construct one. The dict ends up iterate-only: you can't ask for stream 1 / topic 2 / partition 0 directly. A constructor would fix that cheaply. - `src/stats.rs:264` — `cache_metrics` is a `#[getter]`, so every access re-collects the whole map. Iterating the property and indexing it in the same loop is quadratic, which is what the test does at `tests/test_stats.py:66` and `:70`. Building it once in `From<RustStats>` would avoid the surprise. - `tests/test_stats.py:66` — the loop body is the only thing `test_get_stats_cache_metrics_dict` asserts, and on a server with no cached partitions it never runs. Sending and polling messages first, then asserting the map is non-empty, would make it test the conversion. - `tests/test_stats.py:70`-`:73` — `stats.cache_metrics[key] is not None` can't fail (a miss raises `KeyError`), and `>= 0` on `u32` / `u64` is always true. - `tests/test_stats.py:47`-`:49` — exact equality on server-global counters. Fine today since the suite runs serially, but `pytest-xdist` is a declared dev dependency; `>=` avoids the trap. - `src/stats.rs:255` — `1.2.3 -> 100200300` should be `1002003`. Inherited verbatim from `core/common/src/types/stats/mod.rs:73`, so not yours. Fixing it means the Rust doc comment plus a `stub_gen` regen, ideally in `core/common` too. --- This review was drafted by an AI-assisted tool (Apache Magpie), so it may contain mistakes. If you think one of them is misapplied, please reply on the PR, and a maintainer will weigh in. -- 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]
