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

Add option for datastore read rehashing #2904

Merged
merged 5 commits into from
Jun 27, 2016

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Jun 24, 2016

Resolves #2256

Enabled using Datastore.HashOnRead: true in IPFS config.

@Kubuxu Kubuxu force-pushed the feature/repo-runtime-check branch from a5ef1d8 to 39adcb3 Compare June 24, 2016 20:16
@Kubuxu Kubuxu added the need/review Needs a review label Jun 24, 2016
@Kubuxu Kubuxu added this to the ipfs-0.4.3 milestone Jun 24, 2016
@@ -23,6 +23,7 @@ var log = logging.Logger("blockstore")
var BlockPrefix = ds.NewKey("blocks")

var ValueTypeMismatch = errors.New("The retrieved value is not a Block")
var ErrHashMismatch = errors.New("Block on disk has different hash than expected.")
Copy link

Choose a reason for hiding this comment

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

  • the block isn't neccessarily on disk, that's up to the datastore, isn't it? ;)
  • IIRC no dot at the end. @RichardLitt dug up conventions for go error messages the other day

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, right

@ghost
Copy link

ghost commented Jun 24, 2016

Would be good to have an explicit default value for this in repo/config/init.go

License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu Kubuxu force-pushed the feature/repo-runtime-check branch from 39adcb3 to a84c859 Compare June 24, 2016 20:21
bs := bstore.NewBlockstore(n.Repo.Datastore())
n.Blockstore, err = bstore.WriteCached(bs, kSizeBlockstoreWriteCache)

rcfg, err := n.Repo.Config()
Copy link

Choose a reason for hiding this comment

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

I can't comment on non-changed lines, so here: the other n.Repo.Config() in this function can probably be removed.

The error handling is a bit off here too. You're throwing away the error from bstore.WriteCached(), and you can omit the == nil check if you just move that thing to after the usual != nil check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've screwed up here, the err != nil check should be after 134.

Then latter I am checking for err == nil as we are requesting con-fig only in case of online more.

License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu Kubuxu force-pushed the feature/repo-runtime-check branch from c04c1dc to 174ab12 Compare June 24, 2016 20:38
@Kubuxu
Copy link
Member Author

Kubuxu commented Jun 24, 2016

@lgierth thanks for looking at it, I made really stupid mistake there with error handling.

@whyrusleeping
Copy link
Member

lets add a test where we purposely mess with the values in the blocks directory on disk and try to read the data through ipfs.

@whyrusleeping whyrusleeping added need_tests and removed need/review Needs a review labels Jun 26, 2016
@Kubuxu Kubuxu added need/review Needs a review and removed need_tests labels Jun 26, 2016
@@ -0,0 +1,37 @@
#!/bin/sh
#
# Copyright (c) 2016 Mike Pfister
Copy link
Member

Choose a reason for hiding this comment

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

Who is Mike?

Copy link
Member Author

Choose a reason for hiding this comment

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

Someone credited in t0083. 😄

License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu Kubuxu force-pushed the feature/repo-runtime-check branch from e7f4f69 to dead777 Compare June 27, 2016 09:28
@whyrusleeping
Copy link
Member

circleCI seems to be taking a break, LGTM

@whyrusleeping whyrusleeping merged commit 68c87ed into master Jun 27, 2016
@whyrusleeping whyrusleeping deleted the feature/repo-runtime-check branch June 27, 2016 23:09
@whyrusleeping
Copy link
Member

@Kubuxu what was the actual bug resolved here? i think 2256 is incorrect

@Kubuxu
Copy link
Member Author

Kubuxu commented Jun 28, 2016

It was #2259

rehash bool
}

func (bs *blockstore) RuntimeHashing(enabled bool) {
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 not a clear name, because there is runtime hashing regardless
  • use: HashOnRead

@Kubuxu Kubuxu self-assigned this Aug 27, 2016
@Kubuxu Kubuxu removed this from the ipfs-0.4.3-rc1 milestone Sep 12, 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.

4 participants