feat: add proposal #181
feat: add proposal #181
Conversation
| None => match prev_deltas.get(&e_pid) { | ||
| Some(p) => data.extend(&p[..e_off + 1]), | ||
| None => data.extend( | ||
| self.base_space | ||
| .get_slice(e_pid << PAGE_SIZE_NBIT, e_off as u64 + 1)?, | ||
| ), | ||
| }, |
There was a problem hiding this comment.
nit: nearly similar changes 3 times warrants a new function. Something like this (error handling needs work):
| None => match prev_deltas.get(&e_pid) { | |
| Some(p) => data.extend(&p[..e_off + 1]), | |
| None => data.extend( | |
| self.base_space | |
| .get_slice(e_pid << PAGE_SIZE_NBIT, e_off as u64 + 1)?, | |
| ), | |
| }, | |
| None => data.extend(self.from_prev(&e_pid, e_off)), |
and
fn from_prev(&'a self, prev_deltas, page_id, offset) -> &'a [u8] {
match prev_deltas.get(&e_pid) {
Some(p) => &p[..offset + 1],
None => self.base_space.get_slice(page_id << PAGE_SIZE_NBIT, offset as u64 + 1).unwrap(),
}
}There was a problem hiding this comment.
They are not exactly same logic for each case, maybe instead a macro would work?
| fn db_proposal() { | ||
| let cfg = DbConfig::builder().wal(WalConfig::builder().max_revisions(10).build()); | ||
|
|
||
| let db = Db::new("test_db_proposal", &cfg.clone().truncate(true).build()) |
There was a problem hiding this comment.
This needs a tempdir. We seem to do this all over! Maybe we should make a ticket to centralize fetching a test directory, as a method on Db like Db::test("name") that only appears with #[cfg(test)] or something?
| }]; | ||
| let proposal_2 = proposal.clone().propose(batch_2).unwrap(); | ||
| let rev = proposal_2.get_revision(); | ||
| let val = rev.kv_get(b"k"); |
There was a problem hiding this comment.
this repeated code could use a macro:
| let val = rev.kv_get(b"k"); | |
| assert_val!(rev, "k", "v"); |
and
macro_rules! assert_val {
($rev: expr, $key:literal, $expected_val:literal) => {
let actual = $rev.kv_get($key.as_bytes()).unwrap();
assert_eq!(actual, $expected_val.as_bytes().to_vec());
};
}
Thanks! I will address the comment in a follow up. |
|
I prefer using a function in regular code. Macros tend to work better in
test cases
…On Tue, Aug 8, 2023, 12:14 xinifinity ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In firewood/src/storage/mod.rs
<#181 (comment)>:
> + None => match prev_deltas.get(&e_pid) {
+ Some(p) => data.extend(&p[..e_off + 1]),
+ None => data.extend(
+ self.base_space
+ .get_slice(e_pid << PAGE_SIZE_NBIT, e_off as u64 + 1)?,
+ ),
+ },
They are not exactly same logic for each case, maybe instead a macro would
work?
—
Reply to this email directly, view it on GitHub
<#181 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYLR3EOU7H4CGIIS4ZNC73XUKFYJANCNFSM6AAAAAA3DPRUBQ>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
| let mut committed = self.committed.lock(); | ||
| if *committed { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
@xinifinity @rkuris
🤔 , should we change the function signature here so that self is consumed? I think it would make more sense to have a DbError::Invalid_Proposal(Proposal<Store, SharedStore>) that one has to explicitly unwrap if you want to use a proposal that can't be committed.
|
|
||
| if let ProposalBase::Proposal(p) = &self.parent { | ||
| p.commit()?; | ||
| }; |
There was a problem hiding this comment.
This might make the above comment difficult to implement.
There was a problem hiding this comment.
I also think there's an interesting race condition here. We could end up committing the parent, but not committing this proposal... that's unintuitive to me and does not behave like a transaction would.
There was a problem hiding this comment.
After talking to @rkuris, it seems like the result would be the same, so it doesn't really matter which context the proposal-base commit was actually triggered by.
I think it might be a good idea to leave a comment here stating that though.
| m: Arc<RwLock<DbInner<S>>>, | ||
| r: Arc<Mutex<DbRevInner<T>>>, |
There was a problem hiding this comment.
Can we use full names instead of letters, please?
| let parent_root_hash = p.rev.kv_root_hash().ok(); | ||
| let parent_root_hash = | ||
| parent_root_hash.expect("parent_root_hash should not be none"); |
There was a problem hiding this comment.
Why do we call .ok() then .expect. We should just use expect AND add a TODO to handle the error properly
| let parent_root_hash = p.kv_root_hash().ok(); | ||
| let parent_root_hash = | ||
| parent_root_hash.expect("parent_root_hash should not be none"); |
| let kv_root_hash = self.rev.kv_root_hash().ok(); | ||
| let kv_root_hash = kv_root_hash.expect("kv_root_hash should not be none"); |
This commits add the proposal feature which satisfies all the requirements from seasoned milestone for proposal, namely:
On top, you can get a
DbRevfrom theProposal, which you can get the root hash or get (range) proof, etc.