-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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 "ipfs block rm" command. #2962
Conversation
Nice! Subscribing, for the http-api-spec. |
cmds.BoolOption("ignore-pins", "Ignore pins.").Default(false), | ||
}, | ||
Run: func(req cmds.Request, res cmds.Response) { | ||
ignorePins, _, err := req.Option("ignore-pins").Bool() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having this option makes me very nervous. Things break rather spectaculary when we have missing blocks that are pinned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I Know. But I see is as necessary, at least until we can get pinning operations to be O(1). Please also see my initial description on this pr and #2963.
I'm still not comfortable having the option to ignore pins here, letting a single command have the power to corrupt your repo isnt a very good idea. |
@whyrusleeping please have another look. I send the results as a channel and use PostRun now to process the results. I also avoid aborting on a non-fatal error and instead just report that the block can't be deleted and continue on to the next block. I am okay with removing the option to ignore pins, but I want to fix the pin checks so it is |
339a638
to
d7940c8
Compare
Yeah, if we're removing that option then that all looks pretty good. |
d7940c8
to
603fc65
Compare
@whyrusleeping okay, for now I removed the option to ignore pins If you are happy with this fell free to merge. |
1f158c6
to
b39aff6
Compare
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
Provide a new method, Pinner.CheckIfPinned(), which will check if any of the arguments are pinned. Previously IsPinned would need to be called once for each block. The new method will speed up the checking of multiple pinned blocks from O(p*n) to O(p) (where p is the number of pinned blocks and n is the number of blocks to be check) Use the new method in "block rm". License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
b39aff6
to
c88b4b0
Compare
@whyrusleeping do you need anything else for this PR? Looking forward to have #2914 so that ipfs-inactive/js-ipfs-http-client#305 can be finished with regards to the Block API :) |
This looks pretty good to me, it sucks that we have to be so hacky with the commands lib Only thing i'd want to see added here is a test of removing multiple blocks, where a subset of them are pinned. |
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
c278b2c
to
9ae24c3
Compare
@whyrusleeping okay I added a test that deleted both pinned and un-pinned and non-existent blocks and made sure that the valid, un-pinned blocks where removed. @Kubuxu I fixed a bug in the arccache.DeleteBlock() method in bf7c5b3. It seamed a small enough issue that it wasn't worth a separate pull request. |
@kevina i'm not so sure that was a bug in the arccache. I guess it depends on your perspective, if i call delete on something that doesnt exist, should it ever be a problem that it doesnt exist? |
@whyrusleeping the cache changes the behavior of the DeleteBlock() method which does return an error if a key was not found, therefore I consider it a bug. The cache itself relies in DeleteBlock() returning an error if a block was not found. Also I would prefer that "block rm" returns an error if a non-existent block is deleted in case I mistyped the key (or more likely I copy and pasted a key but missed a couple of characters.) If this becomes a blocker I can remove the fix and not test for deleting a non-existent block in my test case. @Kubuxu, your view on this? |
…ocks Add test that removes a combination of pinned, valid, and non-existent blocks in one command. License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
9ae24c3
to
92f6747
Compare
Clarity on what is meant is important:
In general, idempotency is a very important property to have. An op like ipfs block rm --error-if-not-present On Tue, Aug 16, 2016 at 20:49 Kevin Atkinson [email protected]
|
@jbenet the behavior of In other words |
Oh, my bad, I read the diff backwards. On Tue, Aug 16, 2016, 18:49 Kevin Atkinson [email protected] wrote:
|
@jbenet agreed that idempotency is important, But we need to clearly define the expectations of the datastore interface. Currently, all the datastore implementations will return |
This LGTM. I'll wait for @Kubuxu to review as well |
The datastore is not the I guess you could do what rm does and add -f
|
|
@jbenet okay, i thought we were having a discussion about the changes made to the |
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
@jbenet I added a "-f" option. I am of the strong opinion that by default "block rm" should report some sort of error when a block is not found. Doing a quick Google "idempotent delete" brought up this stack overflow question: http://stackoverflow.com/questions/4088350/is-rest-delete-really-idempotent. Multiple people state that idempotency is about side effects not the status code, that is "you can send the request more than once without additional changes to the state of the server". |
Alright, this all LGTM. Thanks a bunch @kevina ! |
This is a basic implementation for #2914.
It accepts multiple blocks to remove, however that does not provide any performance benifit just yet as it calls
pins.IsPinned()
for each block, which traverses every recursive pin to check for indirect pins. This operation costs is O(P*N) where P is the number of pinned blocks and N is the number of blocks to be removed. With a little effort this can be brought down to O(P) by checking for multiple pins at once.Since the pin check is expensive it provides an option to ignore pins. Removing a pinned block will break any command that checks for pins. Right now if this happens recovering from the situation is very difficult. At very least a command should be provided to list any broken or missing pins. An additional optional command can be provided to attempt to repair recursive pins.
TODO:
No longer relevant:
ipfs pin verify
command to list broken or missing pinsOptional and No longer relevant