Replace PartialEnvelope with CommandWithKeys trait#4724
Conversation
| /// partition key range | ||
| #[bilrost(tag(4))] | ||
| #[serde(default)] | ||
| pub partition_key_range: Keys, |
There was a problem hiding this comment.
This is to roll back this change that was introduced in a previous commit. It's not necessary now for all commands to implement HasRecordKeys. This was needed for the write path but it's no longer necessary.
muhamadazmy
force-pushed
the
pr4724
branch
2 times, most recently
from
43412b7 to
e73d761
Compare
|
@AhmedSoliman it might be a good idea to look at #4700 to inspect exactly how the new trait is used in the SelfProposer and friends. |
| // log per partition `Keys::Single` is enough for routing | ||
| // (`Keys::None` would also work); v2 decodes the typed command and | ||
| // uses the full range. | ||
| Command::UpsertRuleBook(_) => Keys::Single(self.partition_key()), |
There was a problem hiding this comment.
I don't think this is entirely correct. This means that bifrost server-side filtering will not deliver this command to the right-hand-side of a partition after the partition is split.
There was a problem hiding this comment.
I see what you mean. The comment might be wrong but the code is still valid since we don't support partition splitting atm. By the time we do we will be already on v2 fully (writing v2) and in that case we will fully honour the full key range.
I think updating the comment should be enough for now, what do you think?
There was a problem hiding this comment.
The assumption you are making here is that splitting will require that all historical commands be on V2. It's a fair assumption but we'll need to make the state machine aware of this.
There was a problem hiding this comment.
I have reworked this so v1 Command body still has the partition key-range but is dropped in v2 but still backward compatible.
Could you take another and let's follow this conversation on #4722 since it's the one that actually introduced the change.
| arena.reserve(cmd.encoded_len()); | ||
| // safe to unwrap because we reserved enough space | ||
| cmd.bilrost_encode(&mut arena).unwrap(); | ||
|
|
## Summary - Convert `UpsertRuleBookCommand` to a `bilrost::Message` carrying `Arc<RuleBook>` directly, instead of a serde struct wrapping an already-bilrost-encoded `Bytes`. Eliminates the previous double encoding (serde-around-bilrost) on the proposal path. - In v1, `Command::UpsertRuleBook` now wraps an opaque `Bytes` payload (the bilrost-encoded `UpsertRuleBookCommand`) rather than the typed struct. Because `UpsertRuleBook` is `*Since v1.7.0` and still unreleased, this payload change is backward compatible. - v1→v2 compatibility no longer decodes/re-encodes the rule book; it forwards the bilrost payload via `Envelope::from_bytes_unchecked`, matching the existing pattern used for `VQueues*` / `VQSchedulerDecisions`. - `record_keys()` for the v1 variant falls back to `Keys::Single(self.partition_key())` since the `partition_key_range` is no longer reachable without decoding. - Leader proposer encodes the command into the shared arena buffer before proposing, avoiding an extra allocation. - State machine now ignores `UpsertRuleBook` messages whose `partition_key_range` does not overlap the local partition's range, mirroring how other range-targeted commands are filtered.
Summary: Replaces the type-erased `PartialEnvelope` with a `CommandWithKeys` trait that unifies how WAL writers obtain record keys for a command: - Auto-implemented for any `Command: HasRecordKeys` (keys come from the command itself). - Implemented for `BodyWithKeys<C>` so commands that don't carry their own keys can still be enriched at the call site. This lets generic write paths accept either form without an intermediate allocation, and removes the now-redundant `StorageDecode` bound from the `Command` trait. As a result: Roll back the changes in the `UpdatePartitionDurabilityCommand` - `UpdatePartitionDurabilityCommand` no longer needs to carry `partition_key_range` (was added in a previous commit) the key range is supplied via `BodyWithKeys` at publish time. (follw up PR to handle the Write path) - `DurabilityTracker` takes a `PartitionId` instead of `Arc<Partition>`, since it no longer needs the key range. (also a rollback)
2 participants
Replace PartialEnvelope with CommandWithKeys trait
Summary:
Replaces the type-erased
PartialEnvelopewith aCommandWithKeystraitthat unifies how WAL writers obtain record keys for a command:
Command: HasRecordKeys(keys come from thecommand itself).
BodyWithKeys<C>so commands that don't carry theirown keys can still be enriched at the call site.
This lets generic write paths accept either form without an intermediate allocation,
and removes the now-redundant
StorageDecodebound from theCommandtrait.As a result:
Roll back the changes in the
UpdatePartitionDurabilityCommandUpdatePartitionDurabilityCommandno longer needs to carrypartition_key_range(was added in a previous commit)the key range is supplied via
BodyWithKeysat publish time.(follw up PR to handle the Write path)
DurabilityTrackertakes aPartitionIdinstead ofArc<Partition>, since it no longer needs the key range.(also a rollback)
Stack created with Sapling. Best reviewed with ReviewStack.