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: RocksDB implementation for File Manager #96

Merged
merged 23 commits into from
Jul 1, 2024

Conversation

a-moreira
Copy link
Member

@a-moreira a-moreira commented Jun 19, 2024

Adds a RocksDB implementation for File Storage.

Next steps in a new PR:

  • Refactor Errors
  • Refactor FileStorage and FileDataTrie interfaces to allow:
    • batch inserting and retrieval.
  • Use concrete types for StorageDb, ditch the Backend trait and thus avoid locking access (RwLock) within the File Manager.

@a-moreira a-moreira marked this pull request as ready for review June 24, 2024 12:57
client/file-manager/src/in_memory.rs Outdated Show resolved Hide resolved
client/file-manager/src/traits.rs Outdated Show resolved Hide resolved

/// Get metadata for a file.
fn get_metadata(&self, key: &HasherOutT<T>) -> Result<FileMetadata, FileStorageError>;

// TODO: check if this method is necessary and what is its use case.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather we simplify the trait api to only include a single insert method. Right now we have insert_file and insert_file_with_data.

IMO we want to abstract away the internal logic and architecture of the file manager from services and tasks. Do you think we could have only an insert_file fn (which does not force services and tasks to create a preliminary trie with FileDataTrie to finally call insert_file_with_data)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this would help making things less confusing, but you'd have to be careful that you can still serve the use case of the RPC of user uploads file, and the BSP receives file handler that gets chunk by chunk.

node/src/service.rs Show resolved Hide resolved
node/src/services/builder.rs Show resolved Hide resolved
client/file-manager/src/rocksdb.rs Outdated Show resolved Hide resolved
client/file-manager/src/rocksdb.rs Show resolved Hide resolved
client/file-manager/src/rocksdb.rs Show resolved Hide resolved
client/file-manager/src/rocksdb.rs Outdated Show resolved Hide resolved
client/file-manager/src/rocksdb.rs Outdated Show resolved Hide resolved
@a-moreira a-moreira marked this pull request as draft June 25, 2024 04:38
@a-moreira a-moreira marked this pull request as ready for review June 26, 2024 18:00
@a-moreira a-moreira requested a review from links234 June 26, 2024 18:12
@a-moreira a-moreira requested a review from snowmead June 26, 2024 21:22
@a-moreira a-moreira force-pushed the feat/file-manager/rocksdb-implementation branch from 3fe1939 to eb3ce4a Compare June 26, 2024 21:39
@a-moreira a-moreira force-pushed the feat/file-manager/rocksdb-implementation branch from 30322b0 to 6563a8c Compare June 27, 2024 13:56
Copy link
Contributor

@snowmead snowmead left a comment

Choose a reason for hiding this comment

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

Good job overall!

Probably most of the comments I left could be put as TODOs, but I think there is some work to
do on the structure of file manager and file trie.

client/file-manager/src/rocksdb.rs Show resolved Hide resolved
client/file-manager/src/rocksdb.rs Show resolved Hide resolved
client/file-manager/src/rocksdb.rs Show resolved Hide resolved
client/file-manager/src/rocksdb.rs Show resolved Hide resolved
client/file-manager/src/rocksdb.rs Show resolved Hide resolved
client/file-manager/src/rocksdb.rs Show resolved Hide resolved
@a-moreira a-moreira requested a review from ffarall June 27, 2024 19:46
.config/nextest.toml Show resolved Hide resolved
node/src/services/builder.rs Show resolved Hide resolved
client/file-manager/src/traits.rs Show resolved Hide resolved

/// Get metadata for a file.
fn get_metadata(&self, key: &HasherOutT<T>) -> Result<FileMetadata, FileStorageError>;

// TODO: check if this method is necessary and what is its use case.
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this would help making things less confusing, but you'd have to be careful that you can still serve the use case of the RPC of user uploads file, and the BSP receives file handler that gets chunk by chunk.

@a-moreira a-moreira merged commit 053486b into main Jul 1, 2024
19 checks passed
@a-moreira a-moreira deleted the feat/file-manager/rocksdb-implementation branch July 1, 2024 03:47
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.

None yet

3 participants