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

rework the graph walking functions with functional options #42

Merged
merged 3 commits into from
Jul 22, 2019

Conversation

MichaelMure
Copy link
Contributor

@MichaelMure MichaelMure commented Jul 18, 2019

This PR:

  • reduce the walk API to 2 simpler functions
  • add consistent and clear control over if the root should be visited
  • add control over the concurrency factor

I also want to implement a IgnoreBadBlock() options to continue walking even when a bad block is read.

Note: FetchGraphWithDepthLimit could be made simpler with a DepthLimit() option as well.

@Stebalien

This commit:
- reduce the API to 2 simpler functions
- add consistent and clear control over if the root should be visited
- add control over the concurrency factor
merkledag.go Outdated
//
// NOTE: It *does not* make multiple concurrent calls to the passed `visit` function.
func WalkParallelDepth(ctx context.Context, getLinks GetLinks, c cid.Cid, startDepth int, visit func(cid.Cid, int) bool) error {
func parallelWalkDepth(ctx context.Context, getLinks GetLinks, root cid.Cid, visit func(cid.Cid, int) bool, options *WalkOptions) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: I kept this function at the same place as before for an easier review, but it would make sense to move it just after sequentialWalkDepth.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

❤️ this is much nicer. I have some suggestions but I'd be happy to merge it as-is if you disagree (no strong opinions).

merkledag.go Outdated
}

links, err := getLinks(ctx, root)
if err != nil {
if err != nil && !options.IgnoreErrors {
Copy link
Member

Choose a reason for hiding this comment

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

So, the idea is that getLinks should this internally. But I agree this option is convenient.

What if we changed this to an error handler (func(cid.Cid, error) error)? That way, we can provide options like:

IgnoreErrors()
IgnoreMissing() // only ignores ipld.ErrorNotFound.
OnMissing(func(c cid.Cid))

Copy link
Contributor Author

@MichaelMure MichaelMure Jul 19, 2019

Choose a reason for hiding this comment

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

Good idea, I'll add a generic OnError(func(err error, c cid.Cid) bool) as well.

merkledag.go Outdated
const defaultConcurrentFetch = 32

// WalkOptions represent the parameters of a graph walking algorithm
type WalkOptions struct {
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this private (or at least make the fields private). Yeah, I know I don't usually do that but that was a mistake.

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

@MichaelMure
Copy link
Contributor Author

It should be good now.

@Stebalien Stebalien merged commit 89dd2ad into ipfs:master Jul 22, 2019
@Stebalien
Copy link
Member

Thanks!

Jorropo pushed a commit to ipfs/boxo that referenced this pull request Mar 15, 2023
…nal-opts

rework the graph walking functions with functional options

This commit was moved from ipfs/go-merkledag@89dd2ad
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.

2 participants