Migrate partition key ranges to KeyRange and extract sharding types#4624
Migrate partition key ranges to KeyRange and extract sharding types#4624AhmedSoliman merged 2 commits intomainfrom
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: e9251889dc
ℹ️ 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".
| impl WithPartitionKey for JournalEntryId { | ||
| fn partition_key(&self) -> PartitionKey { | ||
| self.invocation_id.partition_key() | ||
| } |
There was a problem hiding this comment.
By removing the former blanket impl<T: WithInvocationId> WithPartitionKey for T and only adding point fixes like JournalEntryId, this change drops WithPartitionKey for other public WithInvocationId types (for example ExternalSignalIdentifier and several invocation request/response structs). That is a source-compatible regression for any generic code that previously accepted those types under a WithPartitionKey bound, and we can already see local call sites being rewritten to bypass the trait. If backward compatibility is intended, the remaining WithInvocationId types should get explicit WithPartitionKey impls (or another compatibility mechanism).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The problem is that it creates an awkward circular dependency between the two traits and I'd rather lean on the clean separation of concerns side than the convenience it brings.
AhmedSoliman
force-pushed
the
pr4624
branch
3 times, most recently
from
4238914 to
0188fe4
Compare
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
Thanks for migrating our RangeInclusive<PartitionKey> instances to the new KeyRange @AhmedSoliman. LGTM. +1 for merging :-)
| } | ||
|
|
||
| bilrost::delegate_proxied_encoding!( | ||
| use encoding (bilrost::encoding::Varint) |
There was a problem hiding this comment.
I remember Jack mentioning that the varint encoding of the various protobuf integer fields becomes visible at some point when running Datafusion load tests. Is this something that we need to be aware about here as well? I guess the alternative would be to use Fixed encoding at the cost of often writing an unnecessary byte.
There was a problem hiding this comment.
We are not introducing the new type so we need to maintain wire compatibility. This just moves the type to restate-sharding.
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>
Part of the effort to break the restate-types monolith into composable, focused utility crates. Replace all ~85 uses of std::ops::RangeInclusive<PartitionKey> with the new KeyRange type from restate-util-sharding. Move PartitionId and EqualSizedPartitionPartitioner into restate-util-sharding alongside KeyRange, re-exporting from restate-types for backwards compatibility. The migration is mechanical: .start()/.end() now return u64 by value instead of &u64, .clone() calls are removed (KeyRange is Copy), and construction switches from a..=b syntax to KeyRange::new(a, b). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
Part of the effort to break the restate-types monolith into composable,
focused utility crates.
Replace all ~85 uses of std::ops::RangeInclusive with the new
KeyRange type from restate-util-sharding. Move PartitionId and
EqualSizedPartitionPartitioner into restate-util-sharding alongside KeyRange,
re-exporting from restate-types for backwards compatibility.
The migration is mechanical: .start()/.end() now return u64 by value instead
of &u64, .clone() calls are removed (KeyRange is Copy), and construction
switches from a..=b syntax to KeyRange::new(a, b).
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Stack created with Sapling. Best reviewed with ReviewStack.