Re: [PR] feat(python): expose consumer group create and get APIs [iggy]

["hubcio (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@iggy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org