Conversation
Update Python SDK for the unified proto API: - Enqueue RPC now uses repeated EnqueueMessage/EnqueueResult - BatchEnqueue RPC removed; enqueue_many() replaces batch_enqueue() - Ack/Nack use repeated AckMessage/NackMessage with per-message results - ConsumeResponse only has repeated messages field - BatchMode renamed to AccumulatorMode, BatchEnqueueResult to EnqueueResult - BatchEnqueueError renamed to EnqueueError - Linger.batch_size renamed to Linger.max_messages - Per-message error codes mapped to typed SDK exceptions (e.g. ENQUEUE_ERROR_CODE_QUEUE_NOT_FOUND -> QueueNotFoundError) - All 31 tests pass (16 unit, 15 integration)
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="fila/client.py">
<violation number="1" location="fila/client.py:11">
P2: Remove the unused `EnqueueError` import; with Ruff `F` rules enabled this triggers `F401` and can fail CI.</violation>
</file>
<file name="proto/fila/v1/service.proto">
<violation number="1" location="proto/fila/v1/service.proto:59">
P1: Proto field number reuse: `messages` moved from field 2 → 1, reusing the deleted `message` field's number. During a rolling deployment, an old server writing `repeated messages` on field 2 will be silently ignored by new clients reading field 1. Keep `messages` at field number 2 and reserve field 1 to maintain wire compatibility.</violation>
</file>
<file name="fila/errors.py">
<violation number="1" location="fila/errors.py:33">
P3: `EnqueueError`'s new docstring contradicts runtime behavior: per-message enqueue failures can raise `EnqueueError` via `_map_enqueue_result_error` fallback.</violation>
</file>
<file name="tests/test_batcher.py">
<violation number="1" location="tests/test_batcher.py:67">
P2: The single-flush success test became too weak by only asserting call count; it should assert the actual request content sent to `Enqueue`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| message ConsumeResponse { | ||
| Message message = 1; // Single message (backward compatible, used when batch size is 1) | ||
| repeated Message messages = 2; // Batched messages (populated when server sends multiple at once) | ||
| repeated Message messages = 1; |
There was a problem hiding this comment.
P1: Proto field number reuse: messages moved from field 2 → 1, reusing the deleted message field's number. During a rolling deployment, an old server writing repeated messages on field 2 will be silently ignored by new clients reading field 1. Keep messages at field number 2 and reserve field 1 to maintain wire compatibility.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At proto/fila/v1/service.proto, line 59:
<comment>Proto field number reuse: `messages` moved from field 2 → 1, reusing the deleted `message` field's number. During a rolling deployment, an old server writing `repeated messages` on field 2 will be silently ignored by new clients reading field 1. Keep `messages` at field number 2 and reserve field 1 to maintain wire compatibility.</comment>
<file context>
@@ -6,57 +6,137 @@ import "fila/v1/messages.proto";
message ConsumeResponse {
- Message message = 1; // Single message (backward compatible, used when batch size is 1)
- repeated Message messages = 2; // Batched messages (populated when server sends multiple at once)
+ repeated Message messages = 1;
}
</file context>
| repeated Message messages = 1; | |
| reserved 1; | |
| repeated Message messages = 2; |
There was a problem hiding this comment.
Not applicable — the SDK proto must match the server proto, and the server already uses repeated Message messages = 1; (changed in story 30.1, commit 68e1617). Both server and SDK were updated together in the same epic. Using field 2 with reserved 1 here would create a wire mismatch with the server. Fila is pre-alpha with no rolling deployment guarantees.
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
| from fila.batcher import AutoBatcher, LingerBatcher | ||
| from fila.batcher import AutoAccumulator, LingerAccumulator | ||
| from fila.errors import ( | ||
| EnqueueError, |
There was a problem hiding this comment.
P2: Remove the unused EnqueueError import; with Ruff F rules enabled this triggers F401 and can fail CI.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At fila/client.py, line 11:
<comment>Remove the unused `EnqueueError` import; with Ruff `F` rules enabled this triggers `F401` and can fail CI.</comment>
<file context>
@@ -6,15 +6,16 @@
-from fila.batcher import AutoBatcher, LingerBatcher
+from fila.batcher import AutoAccumulator, LingerAccumulator
from fila.errors import (
+ EnqueueError,
_map_ack_error,
- _map_batch_enqueue_error,
</file context>
There was a problem hiding this comment.
Fixed in aac8edc. Removed the unused EnqueueError import from client.py. Also moved MessageNotFoundError and RPCError to top-level imports (eliminating the inline imports that triggered I001).
There was a problem hiding this comment.
Thanks for the update—appreciate the cleanup!
|
|
||
| assert fut.result(timeout=1.0) == "msg-001" | ||
| stub.Enqueue.assert_called_once_with(proto) | ||
| stub.Enqueue.assert_called_once() |
There was a problem hiding this comment.
P2: The single-flush success test became too weak by only asserting call count; it should assert the actual request content sent to Enqueue.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_batcher.py, line 67:
<comment>The single-flush success test became too weak by only asserting call count; it should assert the actual request content sent to `Enqueue`.</comment>
<file context>
@@ -59,35 +53,33 @@ class TestFlushSingle:
assert fut.result(timeout=1.0) == "msg-001"
- stub.Enqueue.assert_called_once_with(proto)
+ stub.Enqueue.assert_called_once()
def test_rpc_error(self) -> None:
</file context>
| stub.Enqueue.assert_called_once() | |
| stub.Enqueue.assert_called_once() | |
| sent_req = stub.Enqueue.call_args.args[0] | |
| assert len(sent_req.messages) == 1 | |
| assert sent_req.messages[0] == proto |
There was a problem hiding this comment.
Fixed in aac8edc. The test now asserts the actual request content — verifies len(sent_req.messages) == 1 and sent_req.messages[0] == proto.
There was a problem hiding this comment.
Thanks for updating the test to assert the request content!
| and do not raise this exception. This is raised only when the entire | ||
| RPC fails (e.g., network error, server unavailable). |
There was a problem hiding this comment.
P3: EnqueueError's new docstring contradicts runtime behavior: per-message enqueue failures can raise EnqueueError via _map_enqueue_result_error fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At fila/errors.py, line 33:
<comment>`EnqueueError`'s new docstring contradicts runtime behavior: per-message enqueue failures can raise `EnqueueError` via `_map_enqueue_result_error` fallback.</comment>
<file context>
@@ -26,15 +26,31 @@ def __init__(self, code: grpc.StatusCode, message: str) -> None:
- Individual per-message failures are reported via ``BatchEnqueueResult.error``
- and do not raise this exception. This is raised only when the entire batch
+ Individual per-message failures are reported via ``EnqueueResult.error``
+ and do not raise this exception. This is raised only when the entire
RPC fails (e.g., network error, server unavailable).
"""
</file context>
| and do not raise this exception. This is raised only when the entire | |
| RPC fails (e.g., network error, server unavailable). | |
| and in ``enqueue_many()`` do not raise this exception. It is also used as a | |
| fallback for per-message enqueue failures that do not map to a specific type. |
There was a problem hiding this comment.
Fixed in aac8edc. Updated the EnqueueError docstring to note that it is also used as a fallback for per-message enqueue failures that do not map to a more specific type (via _map_enqueue_result_error).
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
1 participant
Summary
BatchEnqueueRPC removed;enqueue_many()replacesbatch_enqueue()EnqueueRequestnow wrapsrepeated EnqueueMessage;EnqueueResponsereturnsrepeated EnqueueResultwith typed error codesAckRequest/NackRequestnow wraprepeated AckMessage/NackMessagewith per-message resultsConsumeResponseonly hasrepeated messages(singularmessagefield removed)BatchModerenamed toAccumulatorMode;BatchEnqueueResulttoEnqueueResult;BatchEnqueueErrortoEnqueueErrorLinger.batch_sizerenamed toLinger.max_messagesEnqueueErrorCodemapped to typed SDK exceptions (e.g.QUEUE_NOT_FOUND->QueueNotFoundError)service.proto;admin.protosyncedTest plan
🤖 Generated with Claude Code
Summary by cubic
Aligns the Python SDK with the unified v1 gRPC API (Story 30.2): removes BatchEnqueue, adds
enqueue_many(), and returns per‑messageEnqueueResultwith typed errors. Also simplifies consume responses and renames batching to accumulation.New Features
enqueue_many(); replacesbatch_enqueue()and returnslist[EnqueueResult].EnqueueErrorCodenow maps to typed SDK exceptions (e.g.,QUEUE_NOT_FOUND->QueueNotFoundError).ConsumeResponseonly includesmessages[](singularmessageremoved).AccumulatorModereplacesBatchMode; backgroundAutoAccumulator/LingerAccumulator.Linger.max_messagesreplacesLinger.batch_size.admin.proto; API key RPCs removed from generated stubs.Migration
batch_enqueue(...)withenqueue_many([...])and handlelist[EnqueueResult].BatchEnqueueResult->EnqueueResult,BatchEnqueueError->EnqueueError,BatchMode->AccumulatorMode.Linger.batch_size->Linger.max_messages.messages[]inConsumeResponse.Written for commit 39a2e2e. Summary will update on new commits.