-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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: add indicator if CID is cached in local datastore #8880
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
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.
The idea looks good but I think that is gonna be too costy.
Files are splited in 256KiB~2MiB blocks.
It might be possible that only a few of them is there, requiring that code to check all the data.
Maybe it's not so bad if that code is server side instead, or we can do a probabilistic thing where if we found random 10 blocks we say we the file.
const notCachedHTML = '<div title="File not cached locally" class="ipfs-_attention"> </div>'; | ||
|
||
listingsPath.forEach((element, index) => { | ||
fetch(element, init) |
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.
AFAIK this request the full file, we dont want to download GiBs just because you openned the dir listing.
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.
- Even with inexpensive
HEAD
application/vnd.ipfs.cache*
check we don't want to send 10k requests when opening big directory like bafybeiggvykl7skb2ndlmacg2k5modvudocffxjesexlod2pfvg5yhwrqm- Even 1k
QmdmQXB2mzChmMeKY47C43LxUdg1NDJ5MWcKMKxDu7RgQm
will be wasteful, if only a small subset is visible on the screen.
- Even 1k
This should be refactored to fetch status only for visible items – see https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API
]; | ||
|
||
const init = { | ||
method: 'GET', |
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.
What we need here is a HEAD
request.
HEAD
is just like a GET
request, but it skips downloading the body of the file, it only sends the headers.
I'm not actually certain this would be work because the server will fetch all directly linked blocks to resolve types and sizes, so without traversal I think every file is gonna show as cached on the gateway because the first block is.
(anyway I would like you to try this pls, as downloading the complete files just to ensure the data is on the server is unacceptable)
92b4b64
to
915a472
Compare
I think this is blocked for now as #8783 is pending. I will move on with requested changes once |
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.
Some notes from go-ipfs sync today:
- This PR is unlikely to be accepted, unless we address performance concerns:
- simple
HEAD
, even if we have basicapplication/vnd.ipfs.cache.block
won't work because dir listing already fetches root blocks of children to tell their size, so every item would be "present". - only other check is to see if entire dag is in cache, and that is expensive
- we would need
application/vnd.ipfs.cache.dag
check that is equivalent to runningipfs dag stat --offline $CID
for every child to see if the entire DAG is present - this is not something public gateways should handle, would be disabled there, but we could enable this on localhost gateway
- we would need
- simple
Overall, a lot of complexity for a little UX indicator, I am afraid.
If you still want to work on this, we need #8783 first, then discuss performance, and then get back to this PR.
const notCachedHTML = '<div title="File not cached locally" class="ipfs-_attention"> </div>'; | ||
|
||
listingsPath.forEach((element, index) => { | ||
fetch(element, init) |
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.
- Even with inexpensive
HEAD
application/vnd.ipfs.cache*
check we don't want to send 10k requests when opening big directory like bafybeiggvykl7skb2ndlmacg2k5modvudocffxjesexlod2pfvg5yhwrqm- Even 1k
QmdmQXB2mzChmMeKY47C43LxUdg1NDJ5MWcKMKxDu7RgQm
will be wasteful, if only a small subset is visible on the screen.
- Even 1k
This should be refactored to fetch status only for visible items – see https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API
Closing this PR, let's continue in #8783 – design decisions need to happen there, which will inform next steps (if any). |
Closes #8817.
Demo
A warning sign appears when the file is not cached locally.
One minor issue is the 404's are printed in the console. That unfortunately cannot be avoided as it is the default behavior of the browser. Let me know if there are changes to be made code or UI wise.