refactor(server,sdk): replace ServerCommand/BytesSerializable with wire types#3041
Open
refactor(server,sdk): replace ServerCommand/BytesSerializable with wire types#3041
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3041 +/- ##
============================================
- Coverage 71.74% 71.07% -0.68%
Complexity 943 943
============================================
Files 1121 1096 -25
Lines 93800 89004 -4796
Branches 71124 66338 -4786
============================================
- Hits 67301 63260 -4041
+ Misses 23863 23342 -521
+ Partials 2636 2402 -234
🚀 New features to boost your workflow:
|
Contributor
|
The cpp test is failing because previously only a 1,000 partitions could be created in a single function call, but that does not seem to be the case anymore. It expected an error when creating 1,001 partitions but did not get one. |
slbotbm
reviewed
Mar 27, 2026
Phase 3 migrated binary handlers to decode wire types, but ShardRequestPayload still wrapped iggy_common command types. Both binary and HTTP handlers had to construct domain types before routing through the shard. ShardRequestPayload now wraps wire types directly. Binary handlers pass decoded wire requests straight through. HTTP handlers construct wire types from JSON-deserialized domain data. Wire-to-domain conversion consolidates in execution.rs, right before EntryCommand construction for WAL persistence. SDK blanket impls now encode requests via WireEncode and decode responses via WireDecode + From/TryFrom conversions in the new wire_conversions module, replacing the 1033-line binary_mapper with type-safe conversions. Also fixes Permissions non-determinism: AHashMap replaced with BTreeMap for streams/topics maps across all crates. This was a latent WAL checksum and consensus bug.
With ShardRequestPayload and SDK using wire types, 25 read-only command structs in iggy_common/commands/ had zero callers. FlushUnsavedBuffer was the last command using send_with_response - migrated to wire types. Removed send_with_response<T: Command>() from the BinaryTransport trait and all three transport impls (TCP, QUIC, WebSocket). The trait no longer requires the Command bound. Deleted ~3500 lines of command structs: GetStream, GetUser, LoginUser, Ping, GetStats, GetSnapshot, GetConsumerOffset, StoreConsumerOffset, and 16 more. Updated CLI commands, HTTP SDK client, server handlers, and prelude re-exports to remove references. Standardized command code constants to use iggy_binary_protocol::codes everywhere (was mixed between iggy_common and binary_protocol).
…o http DTOs EntryCommand serialized domain types via BytesSerializable for WAL persistence while the same types carried serde for HTTP - serving two masters with duplicate codec logic. Wire types in iggy_binary_protocol produce identical bytes but were unused for WAL because the WithId wrappers added server-assigned IDs. Switch EntryCommand variants and WithId wrappers to wire types. Use WireEncode/WireDecode for WAL serialization, codes from iggy_binary_protocol::codes. Remove intermediate domain type construction in execution.rs - wire types flow directly from handler to WAL. Rename commands/ to http/ as pure serde DTOs: strip BytesSerializable, Command trait, Display impls, and roundtrip tests (~5200 lines deleted). Delete the Command trait and duplicate code constants from iggy_common. Migrate the metadata crate's STM macro to decode via WireDecode on wire request types. Fix simulator to use wire types directly. WAL byte format is preserved - WireEncode produces identical bytes to the former BytesSerializable (verified by all state and integration tests).
SDK and server HTTP handlers each defined private copies of the same request/response structs (LoginUser, GetSnapshot, StoreConsumerOffset, etc.) with only Serialize or Deserialize. This duplication meant field additions required coordinated changes in two places. Create 7 shared DTOs in iggy_common::http with both Serialize and Deserialize, then delete the 11 local copies from SDK and server. Also delete the empty types::command module (remnant of the now-removed Command trait). Net -88 lines across 24 files.
BytesSerializable conflated wire protocol encoding with domain type serialization. These 10 impls had no external callers or only contained panicking stubs. Deleted entirely: ClusterMetadata, ClusterNode, ClusterNodeRole, ClusterNodeStatus, TransportEndpoints, TransportProtocol, IggyMessageView, IggyMessageHeaderView, IggyMessagesBatch. Converted PolledMessages::from_bytes to inherent method since it has an active caller in the SDK poll path.
EntryCommand and StateEntry used BytesSerializable for WAL persistence despite the codebase already having WireEncode/ WireDecode in iggy_binary_protocol. This dual-trait situation meant WAL code couldn't benefit from WireEncode's zero-alloc encode() path. EntryCommand now implements both WireEncode and WireDecode. StateEntry implements WireEncode only - from_bytes had zero callers since load_entries() uses hand-rolled async cursor reads for incremental validation and encryption interleaving.
BytesSerializable conflated wire protocol encoding with domain type serialization, duplicating WireEncode/WireDecode which already existed in iggy_binary_protocol. SDK poll/send paths built request bytes through 4-5 intermediate allocations per BytesSerializable::to_bytes() call. SDK poll_messages now constructs PollMessagesRequest directly from wire conversion functions. SDK send_messages uses the zero-copy SendMessagesEncoder with borrowed RawMessage refs. IggyMessage/IggyMessageHeader retain to_bytes/from_bytes as inherent methods for CLI and encryption callers. HashMap header serialization becomes standalone functions since inherent methods cannot be added to foreign types.
The partition count limit (max 1000 per request) was accidentally dropped when command structs were replaced with wire types. The old CreateTopic/CreatePartitions command structs validated this in their Validatable impl, but the new binary handlers received the decoded wire request without checking. Adds MAX_PARTITIONS_PER_REQUEST constant to binary_protocol and validates in both create_topic and create_partitions handlers before forwarding to the control plane.
hubcio
force-pushed
the
refactor-binary-7-http
branch
from
b3d5cb7 to
db9b7df
Compare
The wire type refactor (prior commits) left several issues identified during expert review: - EntryCommand::decode panicked on corrupted WAL entries with inflated length fields instead of returning an error - partitioning_to_wire panicked via unwrap on malformed Partitioning structs reachable through public fields - Permission/identifier conversion code was duplicated between dispatch.rs and wire_conversions.rs (maintenance hazard for wire format divergence) - SDK binary impls erased all WireError context into a generic InvalidFormat, making decode failures opaque - MAX_NAME_LENGTH and MAX_PARTITIONS_COUNT were defined 3 times across HTTP submodules Consolidates wire conversions into iggy_common, adds decode_response helper with tracing, and moves shared constants to lib.rs.
The wire type refactor (prior commits) left several issues identified during 4-expert review: - WithId WireDecode impls panicked on corrupted WAL entries with inflated length fields instead of returning errors. All 5 wrapper types now bounds-check before slicing. - Binary handlers lost domain validation when command structs were replaced with wire types. Username (3-50), password (3-100), PAT name (3-30), and partition count constraints were only enforced on the HTTP path. Restored in 6 handlers. - Topic wire conversion silently degraded unknown compression codes to None via unwrap_or. Changed From to TryFrom with error propagation through SDK callers. - Duplicate 54-line permissions conversion (owned From impl + borrowing free function) collapsed to single delegation.
…ransport codes HTTP SendMessages serialization allocated a HashMap per message using the total batch count as capacity instead of the actual 3 keys per map. A 10K batch wasted ~9GB of heap. Replace HashMap with serde_json::json! macro which builds the object directly without hashing. Also add missing HTTP (3) and WebSocket (4) transport code mappings in wire_conversions, which returned "Unknown" for these protocols.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The binary protocol layer had three redundant serialization
paths: ServerCommand enum + binary_mapper (hand-rolled
command dispatch), BytesSerializable trait (domain type
serialization), and WireEncode/WireDecode (wire protocol
types). All three did the same job with different APIs.
This PR collapses them into one: wire types flow from
transport through shard dispatch to handlers. The
ServerCommand enum, binary_mapper, 45+ command structs,
and BytesSerializable trait are deleted entirely.
Changes:
binary impls, eliminating ServerCommand dispatch
replace with thin HTTP DTOs in iggy_common/http/
WireDecode, delete BytesSerializable from WAL path
trait itself
SendMessagesEncoder (zero-copy) instead of
intermediate BytesSerializable allocations
conversions in wire_conversions.rs