feat: Milestone 1 - added replication plumbing, tests and shared helpers#332
feat: Milestone 1 - added replication plumbing, tests and shared helpers#332jimezesinachi wants to merge 8 commits into
Conversation
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
Test Results362 tests 362 ✅ 19m 20s ⏱️ Results for commit 3029cb0. ♻️ This comment has been updated with latest results. |
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
Benchmark Results |
| DelPred(Vec<u8>), | ||
| DropPredIndex(Vec<u8>), | ||
| DropNonLinearAlgorithmIndex(Vec<u8>), | ||
| DropStore(Vec<u8>), |
There was a problem hiding this comment.
I thought we added PurgeStores recently to DB?
There was a problem hiding this comment.
A'firm, good catch
There was a problem hiding this comment.
Turns out, there appears to be no purge_stores handler for DB. There is one for AI, but none for DB. See here: https://github.com/deven96/ahnlich/blob/main/ahnlich/db/src/server/handler.rs
There was a problem hiding this comment.
Oh schade... we definitely don't have then
Skip for now
| @@ -0,0 +1,425 @@ | |||
| // `clippy::result_large_err` and `clippy::type_complexity` fire on signatures | |||
There was a problem hiding this comment.
Prefix mod with a definition of what the statemachine is and what it does
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
…even96/ahnlich into feat/replication-m1-foundation
| let mut inner = self.inner.lock().expect("state machine lock poisoned"); | ||
| let mut responses = Vec::new(); | ||
|
|
||
| for entry in entries { |
There was a problem hiding this comment.
A bit skeptical about running the entries in a loop to inner.handler.apply. If there's an error with applying entry in the middle, we return error but have already applied some previous entries?
There was a problem hiding this comment.
A bit skeptical about running the entries in a loop to inner.handler.apply. If there's an error with applying entry in the middle, we return error but have already applied some previous entries?
Shey, I see what you're saying. I'm thinking of potential solutions: maybe rollback, or just try and check and fail early somehow?
There was a problem hiding this comment.
Yeah maybe fail early for a start as I'm not sure we have some sure fire way to rollback yet
| source: StorageIOError::read_state_machine(&e), | ||
| })?; | ||
|
|
||
| let mut inner = self.inner.lock().expect("state machine lock poisoned"); |
There was a problem hiding this comment.
Curious about panicking as a whole with respect to lock poisoning of the state machine
Shouldn't it be captured with StorageError instead?
There was a problem hiding this comment.
Curious about panicking as a whole with respect to lock poisoning of the state machine
Shouldn't it be captured with
StorageErrorinstead?
I see what you're saying. I think my thinking here was that if lock is poisoned, the state machine is corrupt and in a way "unrecoverable until node restart". But user API wise, it does make sense to just bubble up as a StorageError, possibly with a message saying "the state machine lock is poisoned"
| opts.create_if_missing(true); | ||
| opts.create_missing_column_families(true); | ||
| let cfs = vec![ | ||
| ColumnFamilyDescriptor::new("logs", Options::default()), |
There was a problem hiding this comment.
Would rather we have static strings referencing the column names and reuse across the rocksdb mod
There was a problem hiding this comment.
Makes sense. I'll use constants
| }) | ||
| } | ||
|
|
||
| fn save_vote_sync(&mut self, vote: &Vote<C::NodeId>) -> Result<(), StorageError<C::NodeId>> { |
There was a problem hiding this comment.
Perhaps a stronger harness around setting&getting meta.... I'm largely wary about having to repeat strings as there's a huge chance that with an update things can be missed
There was a problem hiding this comment.
Makes sense. I'll use helper functions here
| meta: &SnapshotMeta<C::NodeId, C::Node>, | ||
| snapshot: Box<C::SnapshotData>, | ||
| ) -> Result<(), StorageError<C::NodeId>> { | ||
| let cursor: Cursor<Vec<u8>> = (*snapshot).clone().into(); |
There was a problem hiding this comment.
Do we have to clone the entire snapshot in order to deserialize it?
There was a problem hiding this comment.
Looking again, I'm not sure. Will take a closer look and fix
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
No description provided.