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

adds retrieval from cold storage back #34

Merged
merged 2 commits into from
Feb 9, 2024
Merged

Conversation

brunocalza
Copy link
Contributor

@brunocalza brunocalza commented Feb 6, 2024

This PR adds retrieval from cold storage back. Fetching from cold storage is the default behavior of the retrieve command. If you want to fetch from the cache you have to provide true to the --cache flag, e.g. --cache true.

One caveat: because of how we're building the car files now (not wrapping inside a Unix dir), the project https://github.com/ipld/go-car/ does not work for unpacking the car file, you have to use https://github.com/ipfs/go-cid

Signed-off-by: Bruno Calza <brunoangelicalza@gmail.com>
@brunocalza brunocalza self-assigned this Feb 6, 2024
@brunocalza brunocalza marked this pull request as ready for review February 6, 2024 21:22
@sanderpick
Copy link
Member

Maybe a silly question, but would it make sense to by default pull from cache if available, then fallback to cold?

@brunocalza
Copy link
Contributor Author

Maybe a silly question, but would it make sense to by default pull from the cache if available, then fall back to cold?

Yeah, makes sense. This flag implementation comes from the assumption that most of the files wouldn't be stored in the cache (because storing in cache would be more expensive). And if that's true, making a call that most of the time fails would not make sense. But that assumption is probably old. I guess from current architecture thinking all files would go to a DA layer, right? So, checking the cache first would make sense

@dtbuchholz
Copy link
Contributor

Hm yeah agreed.

A related, future feature idea—something else that would be nice is if there was another config file that lets you set default settings or contexts, like the vault name, path to (or value of) the private key, the default retrieval method (cache vs cold), etc. Then, commands that use --private-key, --account (from pk value), --vault, etc. can take these values by default. Idk the best approach for what's in this config file, but my use case is: I'm either creating new vaults and want to use the same settings every time w/o passing as flags, or I'm interacting with the same vault and want to reduce the flags I have to pass.

@sanderpick
Copy link
Member

Maybe a silly question, but would it make sense to by default pull from the cache if available, then fall back to cold?

Yeah, makes sense. This flag implementation comes from the assumption that most of the files wouldn't be stored in the cache (because storing in cache would be more expensive). And if that's true, making a call that most of the time fails would not make sense. But that assumption is probably old. I guess from current architecture thinking all files would go to a DA layer, right? So, checking the cache first would make sense

You're assumption is still correct... we're still on track w/ the expensive cache idea (allowing people to optimize costs by trading off cold storage for cache). But even so, may as well get it from the cache assuming it's faster as long as they are paying for it?

@sanderpick
Copy link
Member

Than again, I don't know how easy it is to check the cache and fallback

@brunocalza
Copy link
Contributor Author

oh okay 👍 Well, which one is faster is tricky, because the cold storage retrieval is not really hitting Filecoin, it's hitting IPFS where the content is pinned by web3 storage. So, at the end of the day is it just as fast as hitting the cache? 😅 but I get your point, theoretically speaking the cold storage should be way slower

it's an easy change. i think it makes sense to do it

return fmt.Errorf("failed to create request: %s", err)
}

if _, err := lassie.Fetch(ctx, request, []types.FetchOption{}...); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

How does lassie know to fetch is from Filecoin (cold storage) and not from IPFS?

Copy link
Contributor Author

@brunocalza brunocalza Feb 7, 2024

Choose a reason for hiding this comment

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

in this setup we don't, it kind of checks multiple protocols until it finds it. but we can specify a specific protocol for retrieval if that's what we want

@@ -625,7 +625,7 @@ func newRetrieveCommand() *cli.Command {
Usage: "Retrieves from cache by setting this flag",
DefaultText: "current directory",
Destination: &cache,
Value: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to this PR, but the current "DefaultText" here is not too relevant 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 catch! but looks like we'll be removing the flag and just rely on cache by default, and if not available cold store

Signed-off-by: Bruno Calza <brunoangelicalza@gmail.com>
@brunocalza brunocalza merged commit 3429c2b into main Feb 9, 2024
2 checks passed
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