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 file requests, files and BSPs storing them #220

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

links234
Copy link
Contributor

@links234 links234 commented Oct 7, 2024

No description provided.

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.

Nice, just a couple of comments.

client/indexer-db/src/models/file.rs Show resolved Hide resolved
} => {
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?;
Copy link
Contributor

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?

@links234 links234 changed the title index file requests, files and BSPs storing them feat: index file requests, files and BSPs storing them Oct 9, 2024
client/indexer-db/src/models/file.rs Outdated Show resolved Hide resolved
client/indexer-db/src/models/peer_id.rs Show resolved Hide resolved
@@ -80,4 +80,15 @@ impl Bucket {
.await?;
Ok(bucket)
}

pub async fn get_by_onchain_bucket_id<'a>(
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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?;
Copy link
Contributor

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 {
Copy link
Contributor

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?

Copy link
Contributor Author

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 {
Copy link
Contributor

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(
Copy link
Contributor

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?

Copy link
Contributor Author

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(
Copy link
Contributor

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 <37149322+ffarall@users.noreply.github.com>
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.

4 participants