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 go-ipfs to use content IDs version 0 #3187

Merged
merged 2 commits into from
Sep 9, 2016
Merged

Conversation

whyrusleeping
Copy link
Member

This is a refactor to replace all object identifiers code with content ID version 0. This is a drop in replacement for the existing key.Key and will not change any outward behaviour. This is just a preparatory refactor for the switchover to CIDv1 and ipld

if err == nil {
for _, block := range bs {
b.bloom.AddTS([]byte(block.Key()))
for _, blk := range good {
Copy link
Member

Choose a reason for hiding this comment

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

This is resolved in #3162

@Kubuxu
Copy link
Member

Kubuxu commented Sep 7, 2016

I went over it, can't find anything wrong with it at first sight but I would love if @lgierth went over it too.

(also it is in conflict with master now, probably due to changes in the blockstore code I pointed out).

License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
@whyrusleeping
Copy link
Member Author

One change i could probably do separately is the Object abstraction in Blockservice. It doesnt need to be done here, but it does simplify things a bit I think. If anyone thinks that it should be done separately I will go ahead and do that.

Other than that, I think that the pin code changes should be looked over closely. @lgierth if you could pay special attention there that would be great :)

@ghost
Copy link

ghost commented Sep 9, 2016

This is a refactor to replace all object identifiers code with content ID version 0.

Is the following accurate? It doesn't replace all usage of key.Key -- from the block layer on inwards we assume raw data (where s/Data/RawData). The layer above that will be the one dealing with the different semantics of various CIDs (soon).

I'm looking over git diff pin/ with this assumption in mind, mh, tricky.


// an Object is simply a typed block
type Object interface {
    Cid() *cid.Cid
    blocks.Block
}

👍

@whyrusleeping
Copy link
Member Author

@lgierth yeah, youre right. the key.Key is still around at the block layer. We're talking about making the CIDs go all the way down to disk, it has its pros and cons, but it would drastically increase the complexity of this change, and is definitely something we can do in a follow up PR.

parent, err := p.dserv.Get(context.Background(), parentKey)
if err != nil {
return err
}
for _, lnk := range parent.Links {
k := key.Key(lnk.Hash)
c := cid.NewCidV0(lnk.Hash)
Copy link

Choose a reason for hiding this comment

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

I assume this is explicitly v0 because we don't wanna start using v1 right now already

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but thinking about it, we could change that to just be a cid.Cast and it would work just fine

Copy link

Choose a reason for hiding this comment

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

Nah it's fine, one silly mistake and we'll have to maintain it :D:D

@ghost
Copy link

ghost commented Sep 9, 2016

LGTM 👍

  • pin/ could see the functions named after Key (e.g. Pinner.DirectKeys()) renamed
  • some of pin/ still uses Key, which is mostly the GC code -- I can understand why you didn't wanna touch it right now, it can get messy and we can take care of it later.
  • these two made it a bit confusing at first, but I think it's good to go

@ghost
Copy link

ghost commented Sep 9, 2016

And I guess this PR has the nice side-effect of reducing the overall dependency on the blocks package :)

@whyrusleeping whyrusleeping merged commit 3babf40 into master Sep 9, 2016
@whyrusleeping whyrusleeping deleted the feat/cidv0 branch September 9, 2016 14:16
@Kubuxu Kubuxu unassigned ghost and Kubuxu Sep 12, 2016
@ghost ghost mentioned this pull request Dec 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants