-
Notifications
You must be signed in to change notification settings - Fork 5
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 file requests, files and BSPs storing them #220
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, just a couple of comments.
} => { | ||
let bsp = Bsp::get_by_onchain_bsp_id(conn, bsp_id.to_string()).await?; | ||
let file = File::get_by_file_key(conn, file_key.as_ref().to_vec()).await?; | ||
BspFile::create(conn, bsp.id, file.id).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we inserting instead of deleting?
@@ -80,4 +80,15 @@ impl Bucket { | |||
.await?; | |||
Ok(bucket) | |||
} | |||
|
|||
pub async fn get_by_onchain_bucket_id<'a>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not specific to this PR, but are we not storing the Bucket root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in this PR, no. I was planning to add root for both buckets and BSPs in another PR.
Is this fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's fine. But we have to be aware that until then we want be able to use the main functionality of our indexer.
pub peer_id: i32, | ||
} | ||
|
||
impl File { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is important to add a get_by_bucket
. That is actually the main reason why we have an indexer in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. You want to add it in this PR when we will also use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add it in this PR if possible
file_key, | ||
new_root: _, | ||
} => { | ||
File::delete(conn, file_key.as_ref().to_vec()).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this deleting the file from the database if just one BSP stops storing it? That would be wrong.
pub file_id: i32, | ||
} | ||
|
||
impl BspFile { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand, this links a File
to a bsp_id
, so that we only store the File
information once, but then we have several BSPs that reference it as being a part of their forests, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
We will have multiple entries here, 1 for each BSP storing the File. Also there are delete triggers setup in Postgres so that if any of the BSP or the File is deleted the relationship entry from this table is also deleted.
/// Table that holds the Files (both ongoing requests and completed). | ||
#[derive(Debug, Queryable, Insertable, Selectable)] | ||
#[diesel(table_name = file)] | ||
pub struct File { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to store the BSPs storing this file? It's one of the main reasons for the indexer.
.await?; | ||
} | ||
pallet_file_system::Event::StorageRequestRevoked { file_key } => { | ||
File::update_step( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we delete it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
.await?; | ||
} | ||
pallet_file_system::Event::StorageRequestExpired { file_key } => { | ||
File::update_step( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it has no BSPs here, we should delete it
Co-authored-by: Facundo Farall <[email protected]>
No description provided.