feat(python): expose partition management by Elioooon · Pull Request #4017 · apache/iggy

Elioooon · 2026-09-01T06:28:49Z

Summary

expose create_partitions and delete_partitions on the Python IggyClient
delegate to the existing Rust PartitionClient methods using the established async binding pattern
add type stubs and an integration test covering partition count changes

Validation

cargo check --manifest-path foreign/python/Cargo.toml
cargo clippy --manifest-path foreign/python/Cargo.toml --all-targets --all-features -- -D warnings
cargo fmt --manifest-path foreign/python/Cargo.toml -- --check
uv run --project foreign/python --no-sync ruff format --check foreign/python/tests/test_partition.py
uv run --project foreign/python --no-sync ruff check foreign/python/tests/test_partition.py
local server: cargo run -p server --bin iggy-server -- --with-default-root-credentials --fresh (with a local macOS hwloc/pkg-config build environment)
uv run --project foreign/python --no-sync pytest foreign/python/tests/test_partition.py -v (1 passed)

Risk boundary

The change is limited to the Python binding surface. It introduces no protocol or server behavior changes and delegates directly to the existing Rust SDK operations. The integration test verifies the observable partition count after both operations.

Disclosure: This contribution was developed with assistance from generative AI. I reviewed the implementation and ran all validation listed above locally.

github-actions · 2026-09-01T06:29:00Z

Thanks for the PR. It is labeled S-waiting-on-review and queued for review.

Slash commands (own line, regular comment) move it around the queue:

/ready - back to S-waiting-on-review after addressing feedback
/author - flip to S-waiting-on-author while you finish changes
/request-review @user-or-team - request a reviewer

See CONTRIBUTING.md for details.

codecov · 2026-09-01T06:59:06Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.98%. Comparing base (bbfad59) to head (8080ce3).
⚠️ Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #4017   +/-   ##
=========================================
  Coverage     84.97%   84.98%           
  Complexity     1402     1402           
=========================================
  Files          1230     1230           
  Lines        181408   181444   +36     
  Branches     147703   147703           
=========================================
+ Hits         154157   154193   +36     
  Misses        23199    23199           
  Partials       4052     4052

Components	Coverage Δ
Rust Core	`86.01% <ø> (ø)`
Java SDK	`67.29% <ø> (ø)`
C# SDK	`75.46% <ø> (ø)`
Python SDK	`90.22% <100.00%> (+0.15%)`	⬆️
PHP SDK	`85.65% <ø> (ø)`
Node SDK	`94.39% <ø> (ø)`
Go SDK	`69.36% <ø> (ø)`

Files with missing lines	Coverage Δ
foreign/python/src/client.rs	`99.86% <100.00%> (+<0.01%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

justinmclean · 2026-09-02T02:12:41Z

Clean, minimal binding. It follows the existing purge_topic shape exactly and does everything #4014 asked for: both methods on the #[pymethods] block, regenerated stubs, and a test that round-trips the count through get_topic. Two observations, neither blocking.

Smaller observations

foreign/python/src/client.rs:791 and :824 — the Raises: block says RuntimeError: If an identifier is invalid or the request fails., but an invalid identifier never reaches the request. impl TryFrom<&PyIdentifier> for Identifier in foreign/python/src/identifier.rs maps both the string and the numeric arm to PyValueError, and TryFrom<PyIdentifier> delegates to it, so create_partitions("", "topic", 2) raises ValueError. create_consumer_group, the next method in this file, documents the same code path correctly:

  /// Raises:
  ///     ValueError: If an identifier is invalid.
  ///     RuntimeError: If the request fails.

Splitting the line the same way and re-running cargo run --bin stub_gen would keep apache_iggy.pyi in step. To be clear about where this came from: it is a file-wide pattern, not something this PR introduced. purge_topic, delete_topic (:731) and get_topics (:628) conflate the two the same way, and get_topic collapses it to a single "raises RuntimeError on failure" line. Fixing the whole file is its own change; matching create_consumer_group in the two new methods is enough here.

foreign/python/tests/test_partition.py:33-41 — this matches Python SDK: expose create_partitions and delete_partitions #4014's test spec exactly, so nothing is missing. One optional strengthening if you feel like it: the count assertions hold whichever partitions the server removed, and Python SDK: expose create_partitions and delete_partitions #4014's design note is specifically that delete_partitions drops the last N. TopicDetails.partitions[].id is exposed and test_topic.py::TestGetTopic::test_get_topic_partitions already reads ids that way, so asserting that the surviving ids are the first N would pin the documented semantics rather than just the arithmetic.

Nothing here blocks the merge.

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.

Elioooon · 2026-09-02T02:33:33Z

@justinmclean Thanks for the careful review. I updated both new methods to document ValueError for identifier conversion separately from request RuntimeError, regenerated and formatted the stub, and strengthened the integration test to assert partition IDs [0, 1, 2, 3] after creation and [0, 1] after deleting the last two. Revalidated Rust fmt/check/clippy, Ruff, and the local-server partition integration test (1 passed).

ethanlin01x

The implementation looks correct to me, but it would be better to add more test coverage.test_partition.py only covers one happy path with string ids. Could you add:

numeric stream/topic ids — the only type conversion here is untested (see test_consumer_group.py:101 for the pattern)
assert the remaining partition ids after delete, not just the count — delete_partitions removes the last N
partitions_count = 0 is rejected on both create_partitions and delete_partitions — the binding does no client-side validation, so this behaviour comes from the server and is worth pinning down
deleting more partitions than the topic has is rejected
missing stream/topic raises RuntimeError

Elioooon · 2026-09-02T04:57:16Z

@ethanlin01x Thanks for the review. Added the requested coverage in a4aab32:

numeric stream/topic IDs exercise both create and delete
surviving partition IDs are asserted after deletion
zero-count create and delete are rejected
deleting more partitions than the topic contains is rejected
missing stream and missing topic are covered for both operations

Revalidated with:

cargo fmt --check
foreign/python/.venv/bin/ruff check foreign/python/tests/test_partition.py
foreign/python/.venv/bin/ruff format --check foreign/python/tests/test_partition.py
local iggy-server --with-default-root-credentials --fresh plus foreign/python/.venv/bin/pytest -q foreign/python/tests/test_partition.py (5 passed)

Elioooon · 2026-09-02T06:08:54Z

/ready

Elioooon · 2026-09-02T06:30:19Z

/ready

Elioooon · 2026-09-03T07:36:29Z

/ready

hubcio

small note on the PR description - the validation block says 1 passed, but the file now has 5 tests, the last four added after that run.

found other problem during review, in server: delete_partitions never rolls the deleted partitions' bytes back out of TopicStats / StreamStats - core/metadata/src/stm/stream.rs:2232 evicts the child entries only - so get_topic, get_stream and /metrics keep counting them until restart. purge does it right via zero_out_all, and delete_topic has the same gap one level up - we will fix it (ignore it in your pr)

hubcio · 2026-09-03T08:53:38Z

+    /// Args:
+    ///     stream_id: Stream identifier as `str | int`.
+    ///     topic_id: Topic identifier as `str | int`.
+    ///     partitions_count: Number of partitions to create.


warning: partitions_count has no documented range. server takes 1..=1000 (MAX_PARTITIONS_PER_REQUEST) and reports 0 as "Too many partitions", so spell it out here - the stub is the only doc a python caller gets.

also at line 819.

hubcio · 2026-09-03T08:53:38Z

+        stream=stream_name, name=topic_name, partitions_count=2
+    )
+
+    with pytest.raises(RuntimeError):


warning: bare pytest.raises(RuntimeError) passes on any failure - every server error maps to RuntimeError here. add match=: "Too many partitions" for zero count, "Invalid partitions count" for over-count, "was not found." for missing stream/topic.

also at lines 84, 100, 117, 119, 121, 123.

hubcio · 2026-09-03T08:53:38Z

+    /// Args:
+    ///     stream_id: Stream identifier as `str | int`.
+    ///     topic_id: Topic identifier as `str | int`.
+    ///     partitions_count: Number of partitions to delete from the end of the topic.


nit: partitions_count drops the "as int" suffix the other args in this same docstring carry. adding it pushes the generated stub line past 88 chars, so rewrap rather than extend.

also at line 785.

hubcio · 2026-09-03T08:53:38Z

+    ///     partitions_count: Number of partitions to delete from the end of the topic.
+    ///
+    /// Returns:
+    ///     An awaitable that resolves to `None` when the partitions are deleted.


nit: the awaitable resolves on metadata commit, not after teardown - the unlink runs later in the reconciler and can back off. say "when the deletion is accepted; storage teardown completes asynchronously".

hubcio · 2026-09-03T08:53:38Z

+        })
+    }
+
+    /// Delete the last partitions from a topic, including all messages stored in them.


nit: neither docstring mentions the consumer group side effect - delete rebalances members off the removed partitions and drops their stats, create leaves the new ones unassigned until the next rebalance. one line on each.

also at line 780.

hubcio · 2026-09-03T08:53:38Z

+
+
+@pytest.mark.asyncio
+async def test_partition_management_rejects_missing_stream_or_topic(


nit: the documented ValueError for a bad identifier is never exercised here, and there's no delete-all-partitions case. skip an over-cap test though - zero and over-cap return the same code, so it would duplicate the zero-count assertion.

hubcio · 2026-09-03T08:53:38Z

+from apache_iggy import IggyClient
+
+
+@pytest.mark.asyncio


nit: module-level test functions - 9 of the 10 other suites group into Test* classes. wrap these in class TestPartitionManagement:.

hubcio · 2026-09-03T08:53:38Z

+
+@pytest.mark.asyncio
+async def test_create_and_delete_partitions(iggy_client: IggyClient, unique_name):
+    stream_name = unique_name()


simplification: the same 7-line create_stream + create_topic preamble is copy-pasted in four tests. pull it into a module-level helper like test_consumer_group.py:37 - there is already a TODO there asking for exactly this.

hubcio · 2026-09-03T08:53:39Z

+        stream=stream_name, name=topic_name, partitions_count=2
+    )
+
+    with pytest.raises(RuntimeError):


simplification: these four raises blocks only differ in which identifier is missing. one parametrize over (method, missing) collapses them and keeps the test name.

hubcio · 2026-09-03T08:53:39Z

+            inner
+                .create_partitions(&stream_id, &topic_id, partitions_count)
+                .await
+                .map_err(|e| PyErr::new::<pyo3::exceptions::PyRuntimeError, _>(e.to_string()))?;


simplification: this makes 38 copies of the same PyErr::new::<PyRuntimeError, _>(e.to_string()) map_err in this file. a to_runtime_error(e: impl ToString) helper, like to_value_error in send_message.rs:93, cuts each site to .map_err(to_runtime_error)?.

also at line 843.

github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label Sep 1, 2026

Elioooon force-pushed the feat/python-partition-management branch from 1230cdc to 86cb5dd Compare September 1, 2026 06:29

jiengup mentioned this pull request Sep 1, 2026

Tracking Issue: Align the Python SDK API with the Rust (native) SDK API #3893

Open

2 tasks

Elioooon force-pushed the feat/python-partition-management branch from 86cb5dd to 54f71d5 Compare September 2, 2026 02:33

ethanlin01x reviewed Sep 2, 2026

View reviewed changes

Comment thread foreign/python/src/client.rs

Comment thread foreign/python/src/client.rs Outdated

Elioooon added 3 commits September 2, 2026 14:28

feat(python): expose partition management

c944ea6

test(python): clarify partition semantics

054dacc

test(python): cover partition management errors

ec33ea7

Elioooon force-pushed the feat/python-partition-management branch from a4aab32 to ec33ea7 Compare September 2, 2026 06:29

docs(python): clarify partition deletion semantics

8080ce3

hubcio requested changes Sep 3, 2026

View reviewed changes



		@pytest.mark.asyncio
		async def test_partition_management_rejects_missing_stream_or_topic(

Uh oh!

Conversation

Elioooon commented Sep 1, 2026

Summary

Validation

Risk boundary

Uh oh!

github-actions Bot commented Sep 1, 2026

Uh oh!

codecov Bot commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

justinmclean commented Sep 2, 2026

Smaller observations

Uh oh!

Elioooon commented Sep 2, 2026

Uh oh!

ethanlin01x left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Elioooon commented Sep 2, 2026

Uh oh!

Elioooon commented Sep 2, 2026

Uh oh!

Elioooon commented Sep 2, 2026

Uh oh!

Elioooon commented Sep 3, 2026

Uh oh!

hubcio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Sep 1, 2026 •

edited

Loading