wal-protocol: store UpsertRuleBookCommand as opaque bilrost bytes in v1#4722
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cbbba9959
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 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.
When a v1 UpsertRuleBook is replayed by a partition whose key range overlaps the encoded command but does not contain the envelope destination key, this Keys::Single makes Bifrost filter the record out before the new overlap check can run: partition processors create readers with KeyFilter::Within(partition_store.partition_key_range()) (crates/worker/src/partition/mod.rs:561-564), and Keys::Single only matches if that exact key is inside the reader range (crates/types/src/logs/mod.rs:214-215). Since the payload still carries the full partition_key_range, use Keys::None or preserve range keys; otherwise overlapping successors/readers can miss a rule-book update entirely.
Useful? React with 👍 / 👎.
muhamadazmy
force-pushed
the
pr4722
branch
2 times, most recently
from
d914f41 to
61c1395
Compare
AhmedSoliman
left a comment
AhmedSoliman
left a comment
There was a problem hiding this comment.
The change looks good to me. Just a small nit to explain to readers that the wrapper is meant to be a v1-only stop-gap measure.
| /// A temporary wrapper for [`UpsertRuleBookCommand`] | ||
| /// to only supply the partition_key_range | ||
| #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] | ||
| pub struct UpsertRuleBookCommandWrapper { | ||
| pub partition_key_range: KeyRange, | ||
| pub rule_book: Bytes, | ||
| /// Bytes are bilrost encoded [`UpsertRuleBookCommand`] | ||
| pub command: Bytes, |
There was a problem hiding this comment.
Can you leave a comment indicating that this is a v1-only wrapper? Or perhaps move to v1/
There was a problem hiding this comment.
Ah it's a good idea indeed to move it to v1.
## 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.
2 participants
wal-protocol: store UpsertRuleBookCommand as opaque bilrost bytes in v1
Summary
UpsertRuleBookCommandto abilrost::MessagecarryingArc<RuleBook>directly, instead of a serde struct wrapping analready-bilrost-encoded
Bytes. Eliminates the previous double encoding(serde-around-bilrost) on the proposal path.
Command::UpsertRuleBooknow wraps an opaqueBytespayload (thebilrost-encoded
UpsertRuleBookCommand) rather than the typed struct.Because
UpsertRuleBookis*Since v1.7.0and still unreleased, thispayload change is backward compatible.
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 toKeys::Single(self.partition_key())since the
partition_key_rangeis no longer reachable without decoding.proposing, avoiding an extra allocation.
UpsertRuleBookmessages whosepartition_key_rangedoes not overlap the local partition's range,mirroring how other range-targeted commands are filtered.
Stack created with Sapling. Best reviewed with ReviewStack.