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

Reprovider strategies #4113

Merged
merged 10 commits into from
Aug 16, 2017
Merged

Reprovider strategies #4113

merged 10 commits into from
Aug 16, 2017

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Aug 1, 2017

Reprovider strategies allow to specify which keys to provide to the content routing.
Supported strategies are: all - all blocks, pinned - all pinned blocks, roots - root/directly pined blocks.
Current reprovider is slow(~0.2keys/s with my setup), so constraining key set should speed the process up, announcing only pin roots should still work quite well in most cases.

TODO:

  • Tests
  • Docs (experimental?)
  • Reduce 'pinned' strategy i/o overhead (it's really bad with larger repos now)
  • Speed up reprovider (probably out of scope of this PR)

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

👍 I really like this, great work so far :) Definitely want to see some tests of different reprovider strategies at least.

@magik6k magik6k force-pushed the feat/reprovider-starts branch 3 times, most recently from fa8524f to 92df0d6 Compare August 3, 2017 10:59
@magik6k
Copy link
Member Author

magik6k commented Aug 3, 2017

Low patch coverage is likely caused by not collecting coverage stats from iptb tests. For example rp.Trigger in https://codecov.io/gh/ipfs/go-ipfs/compare/542d63461ef74a44d586ad525b1954b694b18a56...92df0d60ceac2bf2298480bf3632a8786551806f/src/exchange/reprovide/reprovide.go?before=exchange/reprovide/reprovide.go#L93 is not marked as covered, when it should be (called by ipfs bitswap reprovide)

cc @Kubuxu

@magik6k magik6k force-pushed the feat/reprovider-starts branch 5 times, most recently from b33fbfb to a07e3f9 Compare August 3, 2017 18:58
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
core/core.go Outdated
case "pinned":
keyProvider = rp.NewPinnedProvider(n.Pinning, n.DAG, false)
default:
return fmt.Errorf("unknown reprovider strtaegy '%s'", cfg.Reprovider.Strategy)
Copy link
Member

Choose a reason for hiding this comment

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

typo 'strategy'

docs/config.md Outdated
@@ -15,7 +15,7 @@ a running daemon do not read the config file at runtime.
- [`Identity`](#identity)
- [`Ipns`](#ipns)
- [`Mounts`](#mounts)
- [`ReproviderInterval`](#reproviderinterval)
- [`Reproviderl`](#reprovider)
Copy link
Member

Choose a reason for hiding this comment

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

typo?

}

unmute := rp.muteTrigger()
Copy link
Member

Choose a reason for hiding this comment

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

could I get a comment or two on the muting? I think I get it, but would be great to clearly lay out whats going on

)

var log = logging.Logger("reprovider")

type keyChanFunc func(context.Context) (<-chan *cid.Cid, error)
Copy link
Member

Choose a reason for hiding this comment

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

lets export this type, that way you dont have to redefine it inline up above.

if !onlyRoots {
err := merkledag.EnumerateChildren(ctx, dag.GetLinks, key, set.add)
if err != nil {
return //TODO: propagate to chan / log?
Copy link
Member

Choose a reason for hiding this comment

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

lets definitely not ignore this error, at least log.Error it (though propogating it upwards would be optimal)

}

type streamingSet struct {
set map[string]struct{}
Copy link
Member

Choose a reason for hiding this comment

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

you could reuse the cid.Set for this, it has a visit method that does exactly what you want down in 'add'

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason this is done like this is to reduce i/o hit on merkledag.EnumerateChildren that happens when reprovider starts on large repo (with cid.Set I got 100% io use for about 5min with my repo)

Copy link
Member

Choose a reason for hiding this comment

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

right, what i'm saying is that instead of having set here be a map[string]struct{}, it could instead be a *cid.Set

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, fair point, done

@magik6k magik6k force-pushed the feat/reprovider-starts branch 4 times, most recently from ddaddaf to 5b078b0 Compare August 15, 2017 21:26
@magik6k magik6k force-pushed the feat/reprovider-starts branch 2 times, most recently from 729a3a8 to fcaf2b9 Compare August 15, 2017 22:34
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@whyrusleeping whyrusleeping merged commit dea24ae into master Aug 16, 2017
@whyrusleeping whyrusleeping deleted the feat/reprovider-starts branch August 16, 2017 00:13
@magik6k magik6k added this to the Ipfs 0.4.11 milestone Aug 16, 2017
@magik6k magik6k restored the feat/reprovider-starts branch November 27, 2017 03:34
@magik6k magik6k deleted the feat/reprovider-starts branch November 27, 2017 03:48
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Feb 25, 2022
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Feb 25, 2022
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Mar 4, 2022
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