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(storage-provider): upload a file and download CARv2 #115

Merged
merged 31 commits into from
Jul 8, 2024

Conversation

cernicc
Copy link
Member

@cernicc cernicc commented Jul 4, 2024

Description

The change implements upload and download functionality. Currently, the axum server starts on a different port, but in the future, I imagine we would like to listen on the same address as the general rpc service. There are still many things to be solved before the functionality can be called finished. But as the POC it works.

How do you try it out?

Check #120 for a guide on how to try it out.

Checklist

  • Are there important points that reviewers should know?
  • Make sure that you described what this change does.
  • If there are follow-ups, have you created issues for them?
  • Have you tested this solution?
  • Did you document new (or modified) APIs?

@cernicc cernicc requested a review from th7nder as a code owner July 4, 2024 09:37
@cernicc cernicc linked an issue Jul 4, 2024 that may be closed by this pull request
@cernicc cernicc added the ready for review Review is needed label Jul 4, 2024
@th7nder th7nder changed the title Feat/98/rpc method to upload the file feat(storage-provider): upload a file and download CARv2 Jul 4, 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.

v1 looks great ;D

cli/polka-storage-provider/src/commands/run.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/commands/run.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/commands/run.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/commands/run.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/commands/run.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/commands/run.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/commands/run.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/commands/run.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/storage.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/commands/run.rs Outdated Show resolved Hide resolved
@cernicc cernicc self-assigned this Jul 4, 2024
@cernicc cernicc requested review from th7nder and jmg-duarte July 5, 2024 07:55
@jmg-duarte jmg-duarte added the ready for review Review is needed label Jul 5, 2024
Copy link
Contributor

@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.

I was trying this out and this can't be merged like this.

I thought it was a good idea to have both servers run from a single command but I disagree now. And it's very clear, they're separate concerns...

cargo run -- run doesn't work out of the box, it needs to connect to a substrate server.

The upload server should be a separate command. The change isn't too big and it makes things simpler when handling signals and threads.

The graceful shutdown and timeout still apply, just separate the upload server into another command.

@cernicc
Copy link
Member Author

cernicc commented Jul 5, 2024

Hmm. I wonder how the storage client will exchange the content with the storage provider. Maybe down the line we won't even need the separate server.

@jmg-duarte
Copy link
Contributor

Hmm. I wonder how the storage client will exchange the content with the storage provider. Maybe down the line we won't even need the separate server.

Filecoin does it over Graphsync and the SP is not necessarily on the same machine.

@cernicc cernicc added ready for review Review is needed and removed ready for review Review is needed labels Jul 8, 2024
@cernicc cernicc requested a review from jmg-duarte July 8, 2024 08:48
@cernicc cernicc added ready for review Review is needed and removed ready for review Review is needed labels Jul 8, 2024
cli/polka-storage-provider/src/commands/run.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/commands/run.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/commands/storage.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/storage.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/storage.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Some notes on the docker. I'm not entirely convinced that cargo-chef is really needed since we're going to ship the container to be used, not built from scratch. But if it improves local builds, that works for me

ci/Dockerfile.provider Outdated Show resolved Hide resolved
ci/Dockerfile.provider Outdated Show resolved Hide resolved
ci/PROVIDER.md Outdated Show resolved Hide resolved
ci/PROVIDER.md Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/server.rs Outdated Show resolved Hide resolved
ci/Dockerfile.provider Outdated Show resolved Hide resolved
ci/PROVIDER.md Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/storage.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/storage.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/storage.rs Outdated Show resolved Hide resolved
@cernicc cernicc added ready for review Review is needed and removed ready for review Review is needed labels Jul 8, 2024
@cernicc cernicc requested a review from jmg-duarte July 8, 2024 14:11
Copy link
Contributor

@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.

One last question. It's important because if it fails and it simply stays put, the user will be left clueless.

Copy link
Contributor

@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 job! I learned a bunch about async too

@jmg-duarte jmg-duarte merged commit aa2f5b6 into develop Jul 8, 2024
3 checks passed
@jmg-duarte jmg-duarte deleted the feat/98/rpc-method-to-upload-the-file branch July 8, 2024 15:24
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.

RPC method to upload the file
3 participants