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

blockstore: fix PutMany with cache logic #3162

Merged
merged 3 commits into from
Sep 7, 2016

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Aug 31, 2016

Thanks @whyrusleeping for noticing it.

Removed PutMany logic in bloom cache as it can't help with anything.
Fixed ARC cache to use filtered results instead of all blocks.

Also add test case for it.

@Kubuxu Kubuxu added the status/in-progress In progress label Aug 31, 2016
@whyrusleeping
Copy link
Member

your tests are broken and stuff

// to reduce number of puts we need conclusive infomration if block is contained
// this means that PutMany can't be improved with bloom cache so we just
// just do a passthrough.
return b.blockstore.PutMany(bs)
Copy link
Member Author

@Kubuxu Kubuxu Aug 31, 2016

Choose a reason for hiding this comment

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

I have to add all blocks to bloom....

Will add a test case.

@Kubuxu Kubuxu force-pushed the fix/bs/many-caching-fix branch 3 times, most recently from 515c771 to 6e52419 Compare August 31, 2016 15:28
@Kubuxu Kubuxu added need/review Needs a review and removed status/in-progress In progress labels Aug 31, 2016
@Kubuxu Kubuxu added this to the ipfs 0.4.4 milestone Aug 31, 2016
@Kubuxu
Copy link
Member Author

Kubuxu commented Aug 31, 2016

@whyrusleeping should be fixed, travis is queued up and previously it was having network problems.

Thanks @whyrusleeping for noticing it.

Removed PutMany logic in bloom cache as it can't help with anything.
Fixed ARC cache to use filtered results instad of all blocks.

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

LGTM, thanks for fixing :)

@whyrusleeping whyrusleeping merged commit 6df5afa into master Sep 7, 2016
@whyrusleeping whyrusleeping deleted the fix/bs/many-caching-fix branch September 7, 2016 19:59
@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.

2 participants