Enabling changes for merkle.remove
#647
Conversation
danlaine
requested review from
xinifinity,
rkuris and
richardpringle
as code owners
| if let Some((modified_node, _)) = modified_node { | ||
| Ok(modified_node.clone()) | ||
| } else { | ||
| Err(Error::new(std::io::ErrorKind::Other, "Node not found")) |
There was a problem hiding this comment.
Although I think this code shouldn't be needed at all (see earlier comment) at a minimum this should be the same as when a node is read that has been freed, see read_node in nodestore which does this:
Err(Error::new(
ErrorKind::InvalidData,
"Attempted to read a freed area",
)),
| modified: HashMap<LinearAddress, (Arc<Node>, u8)>, | ||
| // Linear Address -> (Node, Node Size Index) | ||
| // None means the node was deleted. | ||
| modified: HashMap<LinearAddress, Option<(Arc<Node>, u8)>>, |
There was a problem hiding this comment.
I think this is not needed. Why can't you just remove it from modified when it is deleted?
Nobody should ever reference it once it has been deleted in any case.
There was a problem hiding this comment.
What does the delete_node implementation look like without this?
Why can't you just remove it from modified when it is deleted?
What if the node being deleted wasn't modified in this HashedNodeStore? Then removing it from modified would be a no-op.
If the node is in modified, and we delete it, what happens then? When do we tell the underlying node store to delete the node at the given address?
There was a problem hiding this comment.
If the node is in
modified, and we delete it, what happens then? When do we tell the underlying node store to delete the node at the given address?
Yes. We should just delete it from the linear store. If you don't do this, then the space for it can't be reused by another node in the same batch, for example.
There was a problem hiding this comment.
If you don't do this, then the space for it can't be reused by another node in the same batch, for example.
Yeah that's definitely a bug in this PR. Going to close this. I removed these changes from #644
Broken out of #644 to make reviewing easier.
modifiedtoOption, whereNoneindicates the node was deleted.None.