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.

Sign up for GitHub

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

Jump to bottom

Create a write-through block service #3294

Merged
merged 5 commits into from
Oct 12, 2016
Merged

Create a write-through block service #3294

merged 5 commits into from
Oct 12, 2016

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Oct 8, 2016 •
edited
Loading

Create a block service where all writes are guaranteed to go though to the blockstore.

Pull request #3105 (bitswap: don't re-provide blocks we've provided very recently) broke adding files via the filestore (i.e., --no-copy, #2634). The first attempt to fix this was #3253, this is the second.

This also makes BlockService an interface, although in the end it was not strictly required.

@kevina kevina added the status/in-progress In progress label Oct 8, 2016
@kevina kevina changed the title WIP: Create a Write Through block exchange Create a Write Through block exchange Oct 8, 2016
@kevina kevina added the need/review Needs a review label Oct 8, 2016
@kevina kevina mentioned this pull request Oct 8, 2016
Closed
@kevina kevina changed the title Create a Write Through block exchange Create a write through block service Oct 8, 2016
@kevina kevina changed the title Create a write through block service Create a write-through block service Oct 8, 2016
whyrusleeping
whyrusleeping reviewed Oct 9, 2016
View reviewed changes
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.

Think you could add a test or two to make sure the 'writeThrough' happens as expected?

blockservice/blockservice.go
Exchange exchange.Interface
blockstore blockstore.Blockstore
exchange exchange.Interface
// If checkFirst is true than first check that a block doesn't
Copy link
Member

Choose a reason for hiding this comment

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

typo: s/than/then/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@kevina
Copy link
Contributor Author

kevina commented Oct 9, 2016

Think you could add a test or two to make sure the 'writeThrough' happens as expected?

Off hand, I am not sure how. It is not used outside of #2634. Several test in #2634 will fail if the writes don't make it through to the blockstore. In particular, In the backing file moves, the way to update the block is to re-add the file under the new name.

One thing I meant to ask you is if you want to use this by default for the normal add path? This will restore the behavior of readding a file will re-publish it. If this is done there might be a way to test for that, but I might need some help as I am not sure how to test if a block is (re)published.

@kevina kevina mentioned this pull request Oct 9, 2016
Closed
6 tasks
@kevina
Copy link
Contributor Author

kevina commented Oct 10, 2016

@Kubuxu Okay, this is going to need to be rebased. I'll handle that once I get the all clear.

@whyrusleeping does this Look-Good-To-You now?

@kevina
Make BlockService an interface.
dfd5e9a 
License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
kevina and others added 3 commits October 11, 2016 14:57
@kevina
Create a "write through" BlockService.
1173c00 
Create a block service where all writes are guaranteed to go though
to the blockstore.

License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
@Kubuxu @kevina
test: check if NewWriteThrough is not calling Has
9854683 
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu @kevina
Change the test from being Has based to Put based
7e4b74b 
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@kevina kevina removed the status/in-progress In progress label Oct 11, 2016
@kevina
Copy link
Contributor Author

kevina commented Oct 12, 2016

@whyrusleeping rebased and all test are green. Does this look good to you.

whyrusleeping
whyrusleeping requested changes Oct 12, 2016
View reviewed changes
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.

Just one small minor tiny nitpicky change, then i'll merge it. I promise

blockservice/blockservice.go

toput = append(toput, b)
} else {
toput = bs;
Copy link
Member

Choose a reason for hiding this comment

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

semi colons??? noooo

Copy link
Member

Choose a reason for hiding this comment

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

I ran a gofmt and pushed it

@whyrusleeping
run gofmt
affb06d 
License: MIT
Signed-off-by: Jeromy <[email protected]>
@whyrusleeping whyrusleeping merged commit 711cd8f into master Oct 12, 2016
@whyrusleeping whyrusleeping deleted the kevina/dup-prov-cache-fix branch October 12, 2016 14:13
@kevina kevina added this to the Filestore implementation milestone Oct 19, 2016
@ghost ghost mentioned this pull request Dec 23, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whyrusleeping whyrusleeping whyrusleeping approved these changes

Assignees
No one assigned
Labels
need/review Needs a review
Projects
None yet
Milestone
Filestore implementation
Development

Successfully merging this pull request may close these issues.
3 participants
@kevina @whyrusleeping @Kubuxu