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

implement ipfs pin update #3846

Merged
merged 3 commits into from May 18, 2017

Conversation

Projects
None yet
3 participants
@whyrusleeping
Member

whyrusleeping commented Mar 31, 2017

Makes re-pinning a slightly changed dataset much faster

License: MIT
Signed-off-by: Jeromy jeromyj@gmail.com

@whyrusleeping whyrusleeping requested review from kevina and Kubuxu Mar 31, 2017

@whyrusleeping

This comment has been minimized.

Member

whyrusleeping commented Mar 31, 2017

I'll want a solid review of the algorithm here

@Kubuxu

Would be also good to have sharness tests for it.

cmds.StringArg("to-path", true, false, "Path to new object to be pinned."),
},
Options: []cmds.Option{
cmds.BoolOption("unpin", "Remove the old pin.").Default(true),

This comment has been minimized.

@Kubuxu

Kubuxu Mar 31, 2017

Member

What is the use case of update with no unpin?

This comment has been minimized.

@whyrusleeping

whyrusleeping Mar 31, 2017

Member

Its if you want to use the optimized pinning functionality and still keep the old pin

This comment has been minimized.

@Kubuxu

Kubuxu Mar 31, 2017

Member

Ahh, right, because it will piggy bank on the already pinned hashes.

sset := cid.NewSet()
for _, c := range diff {
if c.a == nil {
err := mdag.EnumerateChildrenAsync(ctx, mdag.GetLinksDirect(dserv), c.b, sset.Visit)

This comment has been minimized.

@Kubuxu

Kubuxu Mar 31, 2017

Member

This line is really long

This comment has been minimized.

@whyrusleeping

whyrusleeping Mar 31, 2017

Member

It is really tho?

This comment has been minimized.

@Kubuxu

Kubuxu Mar 31, 2017

Member

aka. too wide for my vim setup.

}
type diffpair struct {
a, b *cid.Cid

This comment has been minimized.

@Kubuxu

Kubuxu Mar 31, 2017

Member

I would rename it to old, new. It clears up a lot.

This comment has been minimized.

@whyrusleeping

whyrusleeping Mar 31, 2017

Member

cant use 'new' lol, its a keyword

This comment has been minimized.

@Kubuxu

Kubuxu Mar 31, 2017

Member

ouch, annoying

a, b *cid.Cid
}
func getLinkDiff(a, b node.Node) []diffpair {

This comment has been minimized.

@Kubuxu

Kubuxu Mar 31, 2017

Member

It took me a minute to properly get what this function does but now I understand.

It diffs links as a set, it doesn't matter for it if some link is repeated.

return err
}
} else {
err := DiffEnumerate(ctx, dserv, c.a, c.b)

This comment has been minimized.

@Kubuxu

Kubuxu Mar 31, 2017

Member

Are we worried here about too deep recursion? Should it be turned into loop with TODO list instead?

This comment has been minimized.

@Kubuxu

Kubuxu Mar 31, 2017

Member

Also coverage shows that this isn't tested at all.

@kevina

This comment has been minimized.

Contributor

kevina commented Mar 31, 2017

@whyrusleeping I would like to review this but it may take up to week before I can get to it.

@kevina

The algo from what I can tell looks solid. I would not use the phrase "edit distance" though as, at least for me, that made understanding what is going on more difficult than it needs to be.

type diffpair struct {
bef, aft *cid.Cid
}

This comment has been minimized.

@kevina

kevina Apr 5, 2017

Contributor

Although it is fairly easy to figure out, I would add a note on the meaning of bef and aft, something along the line of replace bef with aft if bef is nil, than insert aft.

bef, aft *cid.Cid
}
// getLinkDiff returns a changset (minimum edit distance style) between nodes

This comment has been minimized.

@kevina

kevina Apr 5, 2017

Contributor

I was rather confused by what this is doing. From the code it is fairly clear but the phrase "minimum edit distance style" confused me. As someone who wrote a spell checker I was expecting a traditional edit dist algo, and was very confused my the use of the maps. I would probably have had an easier time understanding what is going on if I wasn't trying to reconcile the code with what a traditional string edit dist. algo would be doing.

pin/pin.go Outdated
// Update updates a recursive pin from one cid to another
// this is more efficient than simply pinning the new one and unpinning the
// old one
Update(context.Context, *cid.Cid, *cid.Cid, bool) error

This comment has been minimized.

@kevina

kevina Apr 5, 2017

Contributor

I would name the parameters here. The use of the bool in Update differs from the use in pin and unpin.

@kevina

kevina approved these changes May 13, 2017 edited

Needs a rebase but other then that it LGTM now.

whyrusleeping added some commits Mar 31, 2017

implement ipfs pin update
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
comments and optimize potential rebalances
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
address code review, add comments
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>

@whyrusleeping whyrusleeping merged commit 8e2aed3 into master May 18, 2017

6 of 8 checks passed

codecov/patch 47.31% of diff hit (target 63.82%)
Details
codecov/project 63.82% (-0.01%) compared to 0780a4f
Details
ci/circleci Your tests passed on CircleCI!
Details
codeclimate 3 fixed issues
Details
commit-message-check/gitcop All commit messages are valid
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Kubuxu Kubuxu deleted the feat/pin-update branch Aug 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment