Introduce restate-sharding crate#4623
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Test Results 7 files - 1 7 suites - 1 2m 31s ⏱️ - 1m 13s Results for commit 8ba5af9. ± Comparison against base commit 4e30658. This pull request removes 6 tests.♻️ This comment has been updated with latest results. |
AhmedSoliman
force-pushed
the
pr4623
branch
3 times, most recently
from
c84be1e to
e62afce
Compare
AhmedSoliman
changed the title
AhmedSoliman
changed the title
AhmedSoliman
force-pushed
the
pr4623
branch
3 times, most recently
from
8903621 to
8ba5af9
Compare
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
Thanks for improving the KeyRange abstraction @AhmedSoliman. LGTM. +1 for merging.
| use serde::ser::SerializeStruct; | ||
| use serde::{Deserialize, Deserializer, Serialize, Serializer}; | ||
|
|
||
| /// Serializes as `{"start": <u64>, "end": <u64>}` to match `std::ops::RangeInclusive`'s |
There was a problem hiding this comment.
Some self-describing format might include the KeyRange struct name, I believe. With these formats we might not be wire-compatible wrt std::ops::RangeInclusive.
There was a problem hiding this comment.
I'll add a test to cover this and fix as needed.
| #[test] | ||
| fn bilrost_compat_with_ops_range() { | ||
| use bilrost::{Message, OwnedMessage}; | ||
|
|
||
| // The bilrost crate's existing encoding for RangeInclusive<u64> uses RestateEncoding | ||
| // which is defined in restate-encoding. We can't test cross-type bilrost compat here | ||
| // since RestateEncoding is not available in this crate. That test belongs in | ||
| // restate-encoding's test suite. | ||
| // | ||
| // Here we verify that KeyRange's bilrost encoding is self-consistent. | ||
| #[derive(Debug, PartialEq, bilrost::Message)] | ||
| struct Msg { | ||
| #[bilrost(1)] | ||
| range: KeyRange, | ||
| } | ||
|
|
||
| let msg = Msg { | ||
| range: KeyRange::new(u64::MIN, u64::MAX), | ||
| }; | ||
| let encoded = msg.encode_to_vec(); | ||
| let decoded = Msg::decode(&*encoded).unwrap(); | ||
| assert_eq!(msg, decoded); | ||
| } |
There was a problem hiding this comment.
Seems like a duplicate of bilrost_roundtrip.
There was a problem hiding this comment.
Will be removed in an upcoming PR
Part of the effort to decompose the restate-types monolith into focused, composable utility crates. Add a new `restate-sharding` crate (in `crates/sharding/`) that defines the foundational sharding types: - `KeyRange`: a compact `Copy` newtype over `std::range::RangeInclusive<u64>`. 16 bytes (vs 24 for std::ops::RangeInclusive), wire-format compatible serde and bilrost encoding. Provides iter(), is_overlapping(), and split() helpers. - `PartitionKey`: type alias for u64 - `WithPartitionKey`: trait for types that carry a partition key The crate is re-exported from `restate-types::sharding` for ergonomic access. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Part of the effort to decompose the restate-types monolith into focused,
composable utility crates.
Add a new
restate-shardingcrate (incrates/sharding/) that definesthe foundational sharding types:
KeyRange: a compactCopynewtype overstd::range::RangeInclusive<u64>.16 bytes (vs 24 for std::ops::RangeInclusive), wire-format compatible serde
and bilrost encoding. Provides iter(), is_overlapping(), and split() helpers.
PartitionKey: type alias for u64WithPartitionKey: trait for types that carry a partition keyThe crate is re-exported from
restate-types::shardingfor ergonomic access.Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Stack created with Sapling. Best reviewed with ReviewStack.