Support deletes #68
Support deletes #68
Conversation
sanjayprabhu
force-pushed
the
support_deletes
branch
from
d9a1b62 to
2cf42bb
Compare
src/storage/store/engine.rs
Outdated
| @@ -308,6 +390,15 @@ impl ShardEngine { | |||
| } | |||
| } | |||
|
|
|||
| pub fn sync_id_exists(&mut self, sync_id: &Vec<u8>) -> bool { | |||
There was a problem hiding this comment.
Is this for test use, or public use? If test, I'd go with pub(crate).
Otherwise, probably best to surface the error back to the caller instead of logging and returning false. I think trie.exists should only fail under extreme conditions.
There was a problem hiding this comment.
Test, I'll fix
| @@ -47,6 +47,7 @@ threadpool = "1.8.1" | |||
| rand = "0.8.5" | |||
| humantime-serde = "1.1.1" | |||
| humantime = "2.1.0" | |||
| itertools = "0.13.0" | |||
There was a problem hiding this comment.
don't think this made it into the PR?
There was a problem hiding this comment.
It did. This is what implements into_group_map_by
There was a problem hiding this comment.
Oh, it's one of those things that adds magic behind the scenes. Still getting used to that.
| } | ||
|
|
||
| impl snapchain::ValidatorMessage { | ||
| pub fn fid(&self) -> u32 { |
There was a problem hiding this comment.
Two things:
- I might prefer something like below for improved readability:
impl snapchain::ValidatorMessage {
pub fn fid(&self) -> u32 {
if let Some(fname) = &self.fname_transfer {
return fname.fid as u32;
}
if let Some(event) = &self.on_chain_event {
return event.fid as u32;
}
0
}
}I think upon seeing the unwrap there I really have to scrutinize the code, and it took me a moment to realize that it was indeed safe due to the is_some() above.
I think we can probably remove most of the unwraps() in this file with similar refactors and it will be MUCH faster to reason about, especially at a glance.
- What to do in the fall through case? Should we return an error or even panic? We don't expect this to ever be either? Can we model as actual enum in protobuf?
Maybe 0 is fine, but perhaps a comment? Or a TODO for end state?
There was a problem hiding this comment.
- Ah, much cleaner. Fixed.
- I think if we use oneof it will use enums. We should probably switch to that actually. But even if we did, it won't solve the problem because it's acceptable to have 0 as a value in the protobuf, so callers have to handle fid 0 (which is not valid for the application, fids start from 1). So, I think defaulting to 0 is fine here.
src/storage/store/engine.rs
Outdated
| } | ||
| let mut snap_txns = self.create_transactions_from_mempool(messages); | ||
| for snap_txn in &mut snap_txns { | ||
| self.replay_snap_txn(&snap_txn, txn_batch)?; |
There was a problem hiding this comment.
ok, this is really nice, same logic applied in all three of the main functions.
src/storage/store/engine.rs
Outdated
| } | ||
| } | ||
| let mut snap_txns = self.create_transactions_from_mempool(messages); | ||
| for snap_txn in &mut snap_txns { |
There was a problem hiding this comment.
nit: is snap_txn short for snapchain_txn? I might prefer the longer form as snap_txn implies (on first read) to be introducing a new concept.
| ) -> Result<(), EngineError> { | ||
| match event.body { | ||
| Some(hub_event::hub_event::Body::MergeMessageBody(merge)) => { | ||
| if let Some(msg) = merge.message { |
There was a problem hiding this comment.
Do we expect merge.message to ever be None here? Should we return an error or panic in the missing case, or do you think it's ok or even normal to ignore?
There was a problem hiding this comment.
No, it's possible for it to be None. e.g. registering your fid for the first time will generate only an onchain event, or similarly, registering your fname will only have an fname transfer. Quite normal
| } | ||
| } | ||
| _ => { | ||
| return Err(EngineError::UnsupportedEvent); |
There was a problem hiding this comment.
any value in attaching what kind of event it is, similar to how we handle UnsupportedMessageType?
There was a problem hiding this comment.
This is a placeholder until we implement the other event types, we will not have the fallback case at all. So not worth it. I'll add a todo.
sanjayprabhu
force-pushed
the
support_deletes
branch
from
2cf42bb to
12c596a
Compare
Reflect store operations in the trie and refactor engine to create multiple transactions.