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

feat(): index storage scaffolding & rocksdb implementation #47

Merged
merged 23 commits into from
May 31, 2024

Conversation

cernicc
Copy link
Member

@cernicc cernicc commented May 27, 2024

Description

The PR adds abstraction layer over the specific database implementation used for storage indexing. Rocksdb implementation is also added.

In the rocksdb impl we are using two different column names descriptors. The pieces namespace is used for storing infos of pieces. The cid_infos is used for storing cid infos.

If any additional info would help you as a reviewer please let me know.

Important points for reviewers

Notes

  • The basic implementation will change only if you find any problems or if any bugs are found while writing tests.
  • Still needs implementations from/to bytes for types used. Do you have any suggestion which binary encoder/decoder is good in Rust?

@cernicc cernicc self-assigned this May 27, 2024
@cernicc cernicc marked this pull request as draft May 27, 2024 10:09
@cernicc cernicc linked an issue May 27, 2024 that may be closed by this pull request
@cernicc
Copy link
Member Author

cernicc commented May 28, 2024

Decided to ciborium. Discussion here: https://eig3r.slack.com/archives/C070RN7BZPC/p1716879168517689

@cernicc cernicc added the ready for review Review is needed label May 28, 2024
@cernicc cernicc added ready for review Review is needed and removed ready for review Review is needed labels May 28, 2024
Copy link
Collaborator

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

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

Great starting point!

storage/file-storage/Cargo.toml Outdated Show resolved Hide resolved
storage/file-storage/src/piecestore/rocksdb.rs Outdated Show resolved Hide resolved
storage/file-storage/src/piecestore/rocksdb.rs Outdated Show resolved Hide resolved
storage/file-storage/src/piecestore/rocksdb.rs Outdated Show resolved Hide resolved
storage/file-storage/src/piecestore/rocksdb.rs Outdated Show resolved Hide resolved
storage/file-storage/src/piecestore/rocksdb.rs Outdated Show resolved Hide resolved
storage/file-storage/src/piecestore/rocksdb.rs Outdated Show resolved Hide resolved
storage/file-storage/src/piecestore/rocksdb.rs Outdated Show resolved Hide resolved
storage/file-storage/src/piecestore/rocksdb.rs Outdated Show resolved Hide resolved
storage/file-storage/src/piecestore/types.rs Outdated Show resolved Hide resolved
@cernicc cernicc added ready for review Review is needed and removed ready for review Review is needed labels May 28, 2024
@cernicc cernicc added the ready for review Review is needed label May 29, 2024
jmg-duarte
jmg-duarte previously approved these changes May 29, 2024
Copy link
Collaborator

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

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

IMO, we can merge and develop further as needed.

@cernicc cernicc added ready for review Review is needed and removed ready for review Review is needed labels May 29, 2024
th7nder
th7nder previously approved these changes May 30, 2024
Copy link
Contributor

@th7nder th7nder left a comment

Choose a reason for hiding this comment

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

looks good!

@cernicc cernicc added ready for review Review is needed and removed ready for review Review is needed labels May 30, 2024
Copy link
Collaborator

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

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

LGTM

@cernicc cernicc merged commit 9362c82 into develop May 31, 2024
3 checks passed
@cernicc cernicc deleted the feat/41/index_storage_scaffolding branch May 31, 2024 07:50
Copy link
Contributor

@serg-temchenko serg-temchenko left a comment

Choose a reason for hiding this comment

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

I have some comments, but they are not critical, and this PR has already been merged anyway.

@@ -0,0 +1,3 @@
fn main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please add the crate level doc with text from the readme?

The piecestore module is a simple encapsulation of two data stores, one for PieceInfo and
another for CidInfo.

fn cf_handle(&self, cf_name: &str) -> &ColumnFamily {
self.database
.cf_handle(cf_name)
.expect("column family should be present")
Copy link
Contributor

Choose a reason for hiding this comment

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

Potentially this functionality is abstract enough to treat it as a library, because of which IMO library should not panic, but propagate an error so the client/caller app can decide what to do with it...

Comment on lines +134 to +155
fn add_deal_for_piece(
&self,
piece_cid: &Cid,
deal_info: DealInfo,
) -> Result<(), PieceStoreError> {
// Check if the piece exists
let mut piece_info = self
.get_value_at_key(piece_cid.to_bytes(), PIECES_CF)?
.unwrap_or_else(|| PieceInfo {
piece_cid: *piece_cid,
deals: Vec::new(),
});

// Check if deal already added for this piece
if piece_info.deals.iter().any(|d| *d == deal_info) {
return Err(PieceStoreError::DealExists);
}

// Save the new deal
piece_info.deals.push(deal_info);
self.put_value_at_key(piece_cid.to_bytes(), &piece_info, PIECES_CF)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I have this feeling that we have super abstract functionality which might be separated to a separate crate mixed with business logic related context like here, where we operates with deals on the same layer where we are defining functionality like put_value_at_key etc.

Comment on lines +267 to +269
// verify that getting a piece with a non-existent CID return None
let info = store.get_piece_info(&piece_cid2).unwrap();
assert!(info.is_none(), "expected None, got {:?}", info);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to separate this to a different test case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Review is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indexing storage scaffolding
4 participants