feat(python): expose remaining Topic/TopicDetails fields and partitions#3623
feat(python): expose remaining Topic/TopicDetails fields and partitions#3623mattp5657 wants to merge 8 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3623 +/- ##
============================================
- Coverage 74.11% 71.43% -2.68%
Complexity 937 937
============================================
Files 1262 1286 +24
Lines 132757 136080 +3323
Branches 108647 111880 +3233
============================================
- Hits 98396 97213 -1183
- Misses 31273 35360 +4087
- Partials 3088 3507 +419
🚀 New features to boost your workflow:
|
slbotbm
left a comment
There was a problem hiding this comment.
You've defined MaxTopicSize and IggyExpiry as enums here, but the sdk utilizes normal python ints and timedeltas. Let's change those functions to accept your enums instead of the python ints and structs.
slbotbm
left a comment
There was a problem hiding this comment.
will review in more depth later when I get more time
| /// Use the server's default message expiry. | ||
| ServerDefault(), | ||
| /// Messages expire after this duration. | ||
| ExpireDuration { duration: Py<PyDelta> }, | ||
| /// Messages never expire. | ||
| NeverExpire(), |
There was a problem hiding this comment.
Please also write exactly what does server default and never expire mean. Also, range of values that expireduration accepts
| pub enum MaxTopicSize { | ||
| /// Use the server's default max size. | ||
| ServerDefault(), | ||
| /// The topic is limited to this many bytes. | ||
| Custom { bytes: u64 }, | ||
| /// The topic has no maximum size. | ||
| Unlimited(), | ||
| } |
There was a problem hiding this comment.
Same here. Make the docs explicit. We are doing this since the user cannot trace what exactly these mean without looking at rust sdk code.
|
/author |
This was an oversight from me, thanks for the call out :) |
|
I made the outlined changes, my only concern is if we should continue with backwards compatibility and incorporate it into testing. But wanted to get your thoughts before doing so. :) |
|
/ready |
| match expiry { | ||
| IggyExpiry::ServerDefault() => RustIggyExpiry::ServerDefault, | ||
| IggyExpiry::ExpireDuration { duration } => { | ||
| RustIggyExpiry::ExpireDuration(py_delta_to_iggy_duration(duration)) |
There was a problem hiding this comment.
This also accepts negative duration, which should be rejected as PyValueError
| /// the server deletes the oldest sealed segments to make room for new | ||
| /// messages. | ||
| /// | ||
| /// `bytes` must be greater than zero and less than `u64::MAX`: those two |
There was a problem hiding this comment.
Python callers do not know what u64::MAX means.
|
Fixed both boundary-collision issues flagged in review: negative |
|
/ready |
| Python::attach(|py| { | ||
| let delta = delta1.bind(py); | ||
| let seconds = (delta.get_days() * 60 * 60 * 24 + delta.get_seconds()) as u64; | ||
| let total_seconds = delta.get_days() * 60 * 60 * 24 + delta.get_seconds(); |
There was a problem hiding this comment.
delta.get_days() * 60 * 60 * 24 + delta.get_seconds() is all i32 here (pyo3's get_days/get_seconds both return i32). for |days| >= 24856 (~68yr) the multiply overflows - release wheels wrap silently, debug builds panic. the nasty part is it defeats the negative-duration guard this PR just added right below: a large negative timedelta (easy to get from date arithmetic) wraps to a positive total_seconds, slips past the if total_seconds < 0 check, and a positive expiry gets persisted from a negative input. one-line fix: let total_seconds = i64::from(delta.get_days()) * 86_400 + i64::from(delta.get_seconds()); then range-check before the as u64.
| #[gen_stub(override_type(type_repr = "datetime.timedelta | None", imports=("datetime")))] | ||
| message_expiry: Option<Py<PyDelta>>, | ||
| #[gen_stub(override_type(type_repr = "builtins.int | None"))] max_topic_size: Option<u64>, | ||
| #[gen_stub(override_type(type_repr = "IggyExpiry | None"))] message_expiry: Option< |
There was a problem hiding this comment.
note this is a breaking change for create_topic.message_expiry - it went from timedelta | None to IggyExpiry | None, and that param shipped in the released 0.8.0, so existing callers passing a timedelta now hit a TypeError. the max_topic_size retype and all of update_topic are post-0.8.0 so those break nothing released. worth calling out the message_expiry break, at least in the PR descr.
There was a problem hiding this comment.
Sorry, I left a comment but hadn't updated the PR description yet. I know it's sometimes easier to follow a PR when the description reflects the original push and comments show how the discussion evolved, but I should have been clearer here, apologies.
I see three options:
- Keep the current change. I think it's the most robust approach, but I understand it's a breaking change for
message_expiry. - Revert to the original types used to init the server settings (backwards compatible).
- Accept both, support the old type alongside the new enum.
My main concern is whether we should preserve backwards compatibility here and add test coverage for it, but I wanted to get your thoughts before going down that path.
| /// such by the server. A negative `timedelta` raises `ValueError` when | ||
| /// this value is passed to `create_topic`/`update_topic`. The upper | ||
| /// bound is whatever a `datetime.timedelta` can represent that also fits | ||
| /// in a `u64` microsecond count (about 584,942 years); in practice the |
There was a problem hiding this comment.
this bound isn't accurate today - the timedelta conversion in py_delta_to_iggy_duration overflows i32 at ~68 years, so the real ceiling is far below what's documented. the figure is also a typo: u64 microseconds is ~584,542 years, not 584,942. once the overflow is widened to i64 the ~584,542y bound becomes real and this doc is correct.
| Ok(match max_size { | ||
| MaxTopicSize::ServerDefault() => RustMaxTopicSize::ServerDefault, | ||
| MaxTopicSize::Custom { bytes } => { | ||
| if *bytes == 0 || *bytes == u64::MAX { |
There was a problem hiding this comment.
small consistency thing: MaxTopicSize.Custom(0) (and u64::MAX) reject with ValueError here, but the sibling sentinel IggyExpiry.ExpireDuration(timedelta(0)) silently coerces to server-default instead of rejecting. both are documented so it's not a bug, just worth picking one policy across the two - either both reject the reserved value or both coerce it.
|
|
||
| /// The collection of partitions in the topic. | ||
| #[getter] | ||
| pub fn partitions(&self) -> Vec<Partition> { |
There was a problem hiding this comment.
this getter clones the whole partition vec and builds fresh python objects on every access, so topic.partitions looks like a cheap attribute but re-materializes each time (and the returned objects don't have stable identity). it's a cold metadata path so not a perf concern, but worth a doc note or exposing it as a method so callers don't read it repeatedly in a loop.
| @@ -169,7 +170,9 @@ impl IggyClient { | |||
| } | |||
|
|
|||
| /// Creates a new topic with the given parameters. | |||
There was a problem hiding this comment.
minor: create_topic documents its return and raises as free prose while update_topic below uses the structured Args/Returns/Raises form. worth aligning the two.
| Some(delta) => IggyExpiry::ExpireDuration(py_delta_to_iggy_duration(&delta)), | ||
| None => IggyExpiry::ServerDefault, | ||
| }; | ||
| let expiry = message_expiry |
There was a problem hiding this comment.
this expiry + max_size resolution block is duplicated byte-for-byte in update_topic. could pull it into a small helper (or two) to dedup, no behavior change.
| ("max_topic_size_bytes", "expected_exception"), | ||
| [ | ||
| (-1, OverflowError), | ||
| (2e64, TypeError), |
There was a problem hiding this comment.
the create_topic version of this test also covers (0, ValueError) and (2**64-1, ValueError) for the reserved sentinels, but they're missing here even though update goes through the same try_from. there's also no update_topic test for a negative ExpireDuration like create has. worth mirroring the create cases.
Which issue does this PR address?
Closes #3577
Rationale
Continuation of #3572. That PR exposed
Topic/TopicDetailsbasics but left several fields returned by the server unavailable in Python, so callers couldn't inspect a topic's expiry, size, max size, or its partitions without another SDK.What changed?
TopicandTopicDetailspreviously exposed onlyid,name,partitions_count,compression_algorithm, andreplication_factor, leavingcreated_at,size,message_expiry,max_topic_size, and per-partition data unavailable in Python.Both classes now expose
created_at,size,message_expiry(newIggyExpiryenum), andmax_topic_size(newMaxTopicSizeenum).TopicDetailsalso gainspartitions, a list of newPartitionobjects.Partition(core/common) now derivesCloneto support building that list.Local Execution
AI Usage
Claude was used to help generate and review this PR.