Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pending Part #1

Open
wants to merge 81 commits into
base: master
Choose a base branch
from
Open

Pending Part #1

wants to merge 81 commits into from

Conversation

rongma7
Copy link

@rongma7 rongma7 commented Sep 10, 2024

This change is Reviewable

Copy link
Contributor

@ChenxingLi ChenxingLi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 4 files at r2.
Reviewable status: 1 of 8 files reviewed, 20 unresolved discussions (waiting on @rongma7)

a discussion (no related file):
Fix all the cargo clippy warnings



Cargo.toml line 15 at r1 (raw file):

ethereum-types = "0.12"

in-memory-tree = { path = "src/pending_part" }

move pending part to a module in the crate


src/middlewares/versioned_flat_key_value/mod.rs line 49 at r1 (raw file):

                => { Some(commit) },
            Ok(Some(value)) => { return Ok(value) },
            Err(e) => { return Err(StorageError::PendingError(e)) }

Adjust the orders.


src/middlewares/versioned_flat_key_value/mod.rs line 44 at r2 (raw file):

impl<'db, T: VersionedKeyValueSchema> VersionedStore<'db, T>
where
    T::Key: Hash,

Add Hash to the DBKey


src/middlewares/versioned_flat_key_value/mod.rs line 51 at r2 (raw file):

            Ok(None) => self.pending_part.get_parent_of_root(),
            Err(in_memory_tree::PendingError::CommitIDNotFound(target_commit))
                if target_commit == commit =>

Could target_commit != commit happen?


src/pending_part/src/commit_tree.rs line 10 at r2 (raw file):

type SlabIndex = usize;

pub struct TreeNode<Key: Eq + Hash + Clone, CommitId: Debug + Eq + Hash + Copy, Value: Clone> {

Don't copy <Key: Eq + Hash + Clone, CommitId: Debug + Eq + Hash + Copy, Value: Clone> everywhere.
Define a trait PendingKeyValueSchema, with associated types Key, CommitId, Value.
Define a struct PendingKeyValueConfig<T: VersionedKeyValueSchema>, and implement trait PendingKeyValueSchema.


src/pending_part/src/commit_tree.rs line 45 at r2 (raw file):

    }

    fn commit_id_to_slab_index(

rename to get_xxx_by_xxx


src/pending_part/src/commit_tree.rs line 112 at r2 (raw file):

        let mut slab_indices = vec![subroot_slab_index];
        let mut head = 0;
        while head < slab_indices.len() {

Use VecDeque, and functionpush_back, pop_front


src/pending_part/src/commit_tree.rs line 129 at r2 (raw file):

        let mut path = Vec::new();
        let mut set = HashSet::new();
        while target_node.parent.is_some() {

while let Some(xxx) {...}


src/pending_part/src/commit_tree.rs line 130 at r2 (raw file):

        let mut set = HashSet::new();
        while target_node.parent.is_some() {
            let slab_index = target_node.parent.unwrap();

parent_slab_index


src/pending_part/src/commit_tree.rs line 183 at r2 (raw file):

        let mut target_node = self.slab_index_to_node(target_slab_index);
        let mut commits_rev = HashMap::new();
        loop {

while let Some(xxx) = target_node.parent


src/pending_part/src/commit_tree.rs line 205 at r2 (raw file):

        PendingError<CommitId>,
    > {
        let current_slab_index = self.commit_id_to_slab_index(current_commit_id)?;

why not commit_id_to_node?


src/pending_part/src/commit_tree.rs line 213 at r2 (raw file):

        while current_node.height > target_node.height {
            current_node.current_up(&mut rollbacks);
            current_node = self.slab_index_to_node(current_node.parent.unwrap());

consider implement a function get_parent_node(&self, &TreeNode)->Option<&TreeNode>
and get_parent_node_mut(&mut self, &TreeNode)->Option<&mut TreeNode> if needed.


src/pending_part/src/commit_tree.rs line 270 at r2 (raw file):

    }

    pub fn current_up(&self, rollbacks: &mut HashMap<Key, Option<CommitId>>) {

confusing function name, rename it.


src/pending_part/src/commit_tree.rs line 276 at r2 (raw file):

    }

    pub fn target_up(&self, commits_rev: &mut HashMap<Key, (CommitId, Option<Value>)>) {

confusing function name, rename it.


src/pending_part/src/error.rs line 5 at r1 (raw file):

use thiserror::Error;

#[derive(Debug, PartialEq, Error)]

And Eq


src/pending_part/src/versioned_hash_map.rs line 54 at r1 (raw file):

            let history_inner_map = self.history
                .entry(commit_id)
                .or_insert_with(HashMap::new);

Move history_inner_map outside the loop.


src/pending_part/src/versioned_hash_map.rs line 56 at r1 (raw file):

                .or_insert_with(HashMap::new);
            history_inner_map.insert(key.clone(), value.clone());
            let old_commit_id = {

unnecessary bracket


src/pending_part/src/versioned_hash_map.rs line 107 at r1 (raw file):

            self.current_node = None;
        } else {
            self.walk_to_node_unchecked(target_commit_id.unwrap())?;

use if let Some(xxx) = self.current_node { xxx } else {...}, instead of unwrap


src/pending_part/src/versioned_hash_map.rs line 120 at r1 (raw file):

        } else {
            self.tree
                .lca(self.current_node.unwrap(), target_commit_id)?

use if let Some(xxx) = self.current_node { xxx } else {...}, instead of unwrap

@rongma7 rongma7 force-pushed the pending branch 4 times, most recently from 312b361 to 97c1cb8 Compare September 30, 2024 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants