hubcio commented on code in PR #3581:
URL: https://github.com/apache/iggy/pull/3581#discussion_r3509718095
##########
foreign/python/src/consumer.rs:
##########
@@ -216,6 +220,130 @@ impl IggyConsumer {
}
}
+#[gen_stub_pyclass]
+#[pyclass]
+pub struct ConsumerGroup {
+ pub(crate) inner: RustConsumerGroup,
+}
+
+impl From<RustConsumerGroup> for ConsumerGroup {
+ fn from(group: RustConsumerGroup) -> Self {
+ Self { inner: group }
+ }
+}
+
+#[gen_stub_pymethods]
+#[pymethods]
+impl ConsumerGroup {
+ #[getter]
+ pub fn id(&self) -> u32 {
+ self.inner.id
+ }
+
+ #[getter]
+ pub fn name(&self) -> String {
+ self.inner.name.to_string()
+ }
+
+ #[getter]
+ pub fn partitions_count(&self) -> u32 {
+ self.inner.partitions_count
+ }
+
+ #[getter]
+ pub fn members_count(&self) -> u32 {
+ self.inner.members_count
+ }
+}
+
+#[gen_stub_pyclass]
+#[pyclass]
+pub struct ConsumerGroupDetails {
+ pub(crate) inner: RustConsumerGroupDetails,
+}
+
+impl From<RustConsumerGroupDetails> for ConsumerGroupDetails {
+ fn from(group: RustConsumerGroupDetails) -> Self {
+ Self { inner: group }
+ }
+}
+
+#[gen_stub_pymethods]
+#[pymethods]
+impl ConsumerGroupDetails {
+ #[getter]
+ pub fn id(&self) -> u32 {
+ self.inner.id
+ }
+
+ #[getter]
+ pub fn name(&self) -> String {
+ self.inner.name.to_string()
+ }
+
+ #[getter]
+ pub fn partitions_count(&self) -> u32 {
+ self.inner.partitions_count
+ }
+
+ #[getter]
+ pub fn members_count(&self) -> u32 {
+ self.inner.members_count
+ }
+
+ #[getter]
+ pub fn members(&self) -> Vec<ConsumerGroupMember> {
+ self.inner
+ .members
+ .iter()
+ .map(ConsumerGroupMember::from)
+ .collect()
+ }
+}
+
+#[gen_stub_pyclass]
+#[pyclass]
+pub struct ConsumerGroupMember {
+ pub(crate) inner: RustConsumerGroupMember,
+}
+
+impl From<RustConsumerGroupMember> for ConsumerGroupMember {
Review Comment:
this owned `From<RustConsumerGroupMember>` is dead - the only conversion
site is `members()` at :299 which maps over `.iter()`, so it binds the borrowed
`From<&RustConsumerGroupMember>` at :316. the sibling owned impls
(`ConsumerGroup` :229, `ConsumerGroupDetails` :265) are actually used, just not
this one, and since it's a trait impl `dead_code` won't flag it. safe to delete
these 5 lines. keep the borrowed impl's manual field rebuild though -
`RustConsumerGroupMember` isn't `Clone`, so `member.clone()` isn't an option
there.
##########
foreign/python/src/client.rs:
##########
@@ -243,6 +246,107 @@ impl IggyClient {
})
}
+ /// Create a consumer group for a stream topic.
+ ///
+ /// Args:
+ /// stream_id: Stream identifier as `str | int`.
+ /// topic_id: Topic identifier as `str | int`.
+ /// name: Consumer group name as `str`.
+ ///
+ /// Returns:
+ /// An awaitable that resolves to `None` when the consumer group is
created.
+ ///
+ /// Raises:
+ /// PyRuntimeError: If an identifier is invalid or the request fails.
+
#[gen_stub(override_return_type(type_repr="collections.abc.Awaitable[None]",
imports=("collections.abc")))]
+ fn create_consumer_group<'a>(
+ &self,
+ py: Python<'a>,
+ stream_id: PyIdentifier,
+ topic_id: PyIdentifier,
+ name: String,
+ ) -> PyResult<Bound<'a, PyAny>> {
+ let stream_id = Identifier::try_from(stream_id)?;
+ let topic_id = Identifier::try_from(topic_id)?;
+ let inner = self.inner.clone();
+
+ future_into_py(py, async move {
+ inner
+ .create_consumer_group(&stream_id, &topic_id, &name)
+ .await
+ .map_err(|e| PyErr::new::<pyo3::exceptions::PyRuntimeError,
_>(e.to_string()))?;
+ Ok(())
Review Comment:
optional: `create_consumer_group` drops the `ConsumerGroupDetails` the SDK
returns and resolves to `None`, so a caller who wants the server-assigned id
has to follow up with `get_consumer_group`. this matches how the
`create_stream`/`create_topic` bindings already behave so it's not a regression
- just flagging it if a return-the-details pass ever happens across the whole
create-family.
##########
foreign/python/src/client.rs:
##########
@@ -243,6 +246,107 @@ impl IggyClient {
})
}
+ /// Create a consumer group for a stream topic.
+ ///
+ /// Args:
+ /// stream_id: Stream identifier as `str | int`.
+ /// topic_id: Topic identifier as `str | int`.
+ /// name: Consumer group name as `str`.
+ ///
+ /// Returns:
+ /// An awaitable that resolves to `None` when the consumer group is
created.
+ ///
+ /// Raises:
+ /// PyRuntimeError: If an identifier is invalid or the request fails.
Review Comment:
this `Raises:` (and the same in `get_consumer_group` at :293,
`get_consumer_groups` at :326, plus the generated `.pyi`) says `PyRuntimeError`
for an invalid identifier, but an invalid id comes from `Identifier::try_from`,
which raises `PyValueError` (`identifier.rs:42`/`:45`) synchronously at call
time before the awaitable is even returned. `ValueError` isn't a subclass of
`RuntimeError`, so a caller doing `except RuntimeError` misses it - reachable
with an empty string id. worth splitting into two entries: `PyValueError` for a
bad identifier, `PyRuntimeError` for a request failure, then regen the stub.
##########
foreign/python/src/client.rs:
##########
@@ -28,7 +28,10 @@ use pyo3_stub_gen::derive::{gen_stub_pyclass,
gen_stub_pymethods};
use std::str::FromStr;
use std::sync::Arc;
-use crate::consumer::{AutoCommit, IggyConsumer, py_delta_to_iggy_duration};
+use crate::consumer::{
+ AutoCommit, IggyConsumer, py_delta_to_iggy_duration,
+ ConsumerGroup as PyConsumerGroup, ConsumerGroupDetails as
PyConsumerGroupDetails,
Review Comment:
this import block isn't rustfmt-clean - `rustfmt --edition 2024 --check`
reorders it (the `ConsumerGroup`/`ConsumerGroupDetails` imports sort up before
`IggyConsumer`). it slips through because `foreign/python` is in the workspace
`exclude`, so `cargo fmt --all` skips the crate and there's no separate rustfmt
gate for it the way `foreign/php` has one. running rustfmt fixes the block, and
it'd be worth adding a fmt check scoped to `foreign/python` so this stops
shipping uncaught.
--
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]
