-
Notifications
You must be signed in to change notification settings - Fork 1
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
Conversation
Signed-off-by: Bruno Calza <brunoangelicalza@gmail.com>
Maybe a silly question, but would it make sense to by default pull from cache if available, then fallback 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 |
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 |
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? |
Than again, I don't know how easy it is to check the cache and fallback |
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 { |
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.
How does lassie
know to fetch is from Filecoin (cold storage) and not from IPFS?
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.
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, |
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.
unrelated to this PR, but the current "DefaultText" here is not too relevant 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 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>
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 providetrue
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