Remove the PartitionStorage, StateStorage and StateReader abstraction #1878
Conversation
| @@ -140,6 +140,7 @@ impl ReadOnlyJournalTable for PartitionStore { | |||
| invocation_id: &InvocationId, | |||
| journal_index: u32, | |||
| ) -> Result<Option<JournalEntry>> { | |||
| self.assert_partition_key(invocation_id); | |||
There was a problem hiding this comment.
Amazing that those live here now!
| @@ -347,20 +182,17 @@ impl<Codec> StateMachine<Codec> { | |||
| } | |||
| } | |||
|
|
|||
| pub(crate) struct StateMachineApplyContext<'a, S> { | |||
| storage: &'a mut S, | |||
| pub(crate) struct StateMachineApplyContext<'a, 'b: 'a> { | |||
There was a problem hiding this comment.
given we have two lifetimes, maybe give them meaningful names?
Also I'm not so sure you need two lifetimes in this Context struct, isn't this reference going to live as long as the mut borrow to transaction anyway for the usage of this struct?
There was a problem hiding this comment.
We need two lifetimes because we need to tell Rust that this exclusive borrow will be shorter than the lifetime of the transaction.
| pub(crate) struct StateMachineApplyContext<'a, S> { | ||
| storage: &'a mut S, | ||
| pub(crate) struct StateMachineApplyContext<'a, 'b: 'a> { | ||
| storage: &'a mut PartitionStoreTransaction<'b>, |
There was a problem hiding this comment.
Not sure I agree with replacing with the concrete type here. I think it's still useful to use traits in case we wanna test the individual methods with a simple unit test that doesn't require rocksdb (or if we wanna replace the rocksdb impl with an in memory one later, always for testing)
There was a problem hiding this comment.
Same comment for essentially all the rest of the file, I feel we should still keep the trait usages, and just use the Table traits
There was a problem hiding this comment.
I guess we can keep it.
There was a problem hiding this comment.
Well there was a reason not do it because it is super annoying to add the different tables.
tillrohrmann
force-pushed
the
issues/276
branch
from
f1b65f4
to
6976401
Compare
|
I've reintroduced the generic nature of the |
tillrohrmann
force-pushed
the
issues/276
branch
from
6976401
to
9fdbed7
Compare
|
I'd be in favor of removing unnecessary generics and only introduce them when needed. |
This commit removes the PartitionStorage, StateStorage and StateReader abstraction and replaces them with the PartitionStore. This removes an unnecessary layer of abstraction which is no longer needed. This fixes restatedev#276.
tillrohrmann
force-pushed
the
issues/276
branch
from
9fdbed7
to
c2b013f
Compare
|
I would leave further clean-up work as follow-ups to this PR. |
This commit removes the PartitionStorage, StateStorage and StateReader abstraction and replaces them with the PartitionStore. This removes an unnecessary layer of abstraction which is no longer needed.
This fixes #276.