"block rm": move core functionally into blockstore_util package #3225
Conversation
kevina
force-pushed
the
kevina/rm-block-refactor
branch
from
c36cee4
to
973076b
Compare
|
I refactored the "block rm" code so I can reuse it in #2634. |
kevina
force-pushed
the
kevina/rm-block-refactor
branch
from
973076b
to
c8269c1
Compare
kevina
changed the title
|
The teamcity failure looks completely unrelated to my change... |
|
@whyrusleeping I would like to get this merged ASAP as moving code around is guaranteed to produce conflicts in the future. |
| } | ||
|
|
||
| func RmBlocks(blocks bs.GCBlockstore, pins pin.Pinner, out chan<- interface{}, cids []*cid.Cid, opts RmBlocksOpts) error { | ||
| go func() { |
There was a problem hiding this comment.
This feels weird that the function returns an error, but all it does is spawn a goroutine and returns nil...
There was a problem hiding this comment.
It returns an error in future code that is not ready to be merged. See https://github.com/ipfs-filestore/go-ipfs/blob/c27121b429e8c136e50225f9c07c986a44fbad84/blocks/blockstore/util/remove.go#L30.
| } else { | ||
| out <- &RemovedBlock{ | ||
| Hash: r.Key.String(), | ||
| Error: r.String(), |
There was a problem hiding this comment.
Is Error being overloaded to contain other information?
There was a problem hiding this comment.
No. r.String() returns a message as to why the block is pinned. If RemovedBlock returns with the Error field it means the block could not be removed.
|
|
||
| func (err RmError) Error() string { return err.Msg } | ||
|
|
||
| func ProcRmOutput(in <-chan interface{}, sout io.Writer, serr io.Writer) *RmError { |
There was a problem hiding this comment.
return type here should just be error. Otherwise you get into weird issues where:
var err error
err = ProcRmOutput(...)
// err is never nil
There was a problem hiding this comment.
The return type is a pointer to RmError which implemets the error interface and can (and does) return nil. Nevertheless I no longer require the extra information that RmError returns so I removed that structure and just use a normal error now.
There was a problem hiding this comment.
@kevina I understand, The issue with the (old) code is this: https://play.golang.org/p/0YxTEK55zb
|
What are we accomplishing by moving this out to a separate package? |
License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>
Note: this code can not go in the "blockstore" package due to a circular dependency with the "pin" package. License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>
License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>
kevina
force-pushed
the
kevina/rm-block-refactor
branch
from
c8269c1
to
6b2b976
Compare
License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>
|
@whyrusleeping to answer your direct question, this code is it's own package because I am not sure where else to put it. The code can not go in the blockstore package because that will create a circular dependency with the "pin" package. The refactoring is necessary so that I can reuse it in #2634 For example in |
|
@kevina okay, that sounds good to me. |
ghost
mentioned this pull request
No description provided.