Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fail to construct preload hamt shards when traversal fails #55

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

willscott
Copy link
Collaborator

Fix #54

@willscott willscott requested review from rvagg and aschmahmann July 6, 2023 23:01
func (n UnixFSHAMTShard) Length() int64 {
len, err := n.length()
if err != nil {
return 0
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 feel strongly about the number chosen here, as mentioned in ipld/go-ipld-prime#531 we're really in a rough spot here.

This is a behavior change from previously, but if you don't want to return -1 or a different negative number then 0 is probably more reasonable than what we were doing here before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i'm happy to take suggestions - do you think it makes sense to do -2 or similar in this case?

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer 0 until we have some kind of agreement on the best way to deal with ipld/go-ipld-prime#531, otherwise this is an outlier.

@rvagg
Copy link
Member

rvagg commented Jul 7, 2023

We could memoize the err inside UnixFSHAMTShard so the next call to a method that returns an error can return it. Unfortunately, by saying 0 to the Length() call we're minimising the chance that another method is going to be called on it.

@willscott
Copy link
Collaborator Author

given that 0 length leaves us unlikely to get the memoized error, i think i'm going to not add the complexity for that.

@willscott willscott merged commit a731afe into main Jul 21, 2023
willscott added a commit that referenced this pull request Jul 21, 2023
including the fix from #55
@willscott willscott mentioned this pull request Jul 21, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HAMT Preloader does not return error even if blocks do not load
3 participants