-
-
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
[WIP] Filestore Implementation #2634
Conversation
7edf207
to
da43103
Compare
@@ -19,48 +20,61 @@ type Block interface { | |||
Loggable() map[string]interface{} | |||
} | |||
|
|||
// Block is a singular block of data in ipfs | |||
type RawBlock struct { | |||
type BasicBlock struct { |
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.
aww, renaming this from RawBlock
to BasicBlock
makes the diff so noisy... I was hoping to avoid that by getting that change merged separately
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.
Sorry, I missed that change as it was in a separate commit. Let me know if you know if you want me to do anything about it, like submitting this change separately.
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.
@whyrusleeping, this rename is only in one file, it is exported because I might eventually refactor my Filestore specific changes to blocks/blocks.go into another file.
I imagine that this is going to take a while before it gets merged. So for now I think it will be best to maintain this as a separate fork while the goal of eventually merging everything in. @whyrusleeping I will still like to work with you on API issues and work to get major API changes merged first to avoid stagnation. I have created a README for the new filestore available here https://github.com/ipfs-filestore/go-ipfs/blob/kevina/filestore/filestore/README.md some notes on my fork are available here https://github.com/ipfs-filestore/go-ipfs/wiki. @jefft0 if you are interested I could use some serious testing now. Others all of course welcome to test it out. |
} | ||
|
||
func NewReaderFile(filename, path string, reader io.ReadCloser, stat os.FileInfo) *ReaderFile { | ||
return &ReaderFile{filename, path, reader, stat} | ||
func NewReaderFile(filename, path, abspath string, reader io.ReadCloser, stat os.FileInfo) *ReaderFile { |
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.
this can probably be changed so that it only accepts the absolute path instead of a filename, path and absolute path.
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.
(and a similar change can be made throughout this part of the codebase)
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.
@whyrusleeping I added absolute path, because I was unsure how FullPath was used in the code base. Is it acceptable for full path to be an absolute path? I am unsure how FullPath and "add -r" interact.
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.
To be honest, i'm not entirely sure either. But that said, our tests in that area are quite thorough, If you change something, go into test/sharness
, run make deps
and then run ./t0040-add.sh
, and maybe some of the other t004*
tests too to make sure nothing weird happened
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.
All right I will start by trying to change FullPath to an absolute path add see what happens when used with "add -r". I will make sure the contents of the directory objects are correct.
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.
It turns our that when adding directories the directory is part of the filename. For example after adding some debug output here is the result of one of the test:
expecting success:
mkdir mountdir/planets &&
echo "Hello Mars!" >mountdir/planets/mars.txt &&
echo "Hello Venus!" >mountdir/planets/venus.txt &&
ipfs add -r mountdir/planets >actual
Filename: planets/mars.txt
Path: mountdir/planets/mars.txt
Filename: planets/venus.txt
Path: mountdir/planets/venus.txt
ok 42 - 'ipfs add -r' succeeds
Thus we still need to pass in both a filename and a path. It seams okay to just make the path absolute, so I will do that I remove the change that adds an absolute path to the various parts in the files
package.
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.
We could extract the filename from the passed in path though
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 tried that and it is not going to work. This test will fail. The filename needs to be "planets/mars.txt" if you try to extract it it will be just "mars.txt". You will get this:
expecting success:
mkdir mountdir/planets &&
echo "Hello Mars!" >mountdir/planets/mars.txt &&
echo "Hello Venus!" >mountdir/planets/venus.txt &&
ipfs add -r mountdir/planets >actual
ok 42 - 'ipfs add -r' succeeds
expecting success:
PLANETS="QmWSgS32xQEcXMeqd3YPJLrNBLSdsfYCep2U7CFkyrjXwY" &&
MARS="QmPrrHqJzto9m7SyiRzarwkqPcCSsKR2EB1AyqJfe8L8tN" &&
VENUS="QmU5kp3BH3B8tnWUU2Pikdb2maksBNkb92FHRr56hyghh4" &&
echo "added $MARS planets/mars.txt" >expected &&
echo "added $VENUS planets/venus.txt" >>expected &&
echo "added $PLANETS planets" >>expected &&
test_cmp expected actual
> diff -u expected actual
--- expected 2016-05-11 03:20:12.742743658 +0000
+++ actual 2016-05-11 03:20:12.466738156 +0000
@@ -1,3 +1,2 @@
-added QmPrrHqJzto9m7SyiRzarwkqPcCSsKR2EB1AyqJfe8L8tN planets/mars.txt
-added QmU5kp3BH3B8tnWUU2Pikdb2maksBNkb92FHRr56hyghh4 planets/venus.txt
-added QmWSgS32xQEcXMeqd3YPJLrNBLSdsfYCep2U7CFkyrjXwY planets
+added QmPrrHqJzto9m7SyiRzarwkqPcCSsKR2EB1AyqJfe8L8tN mars.txt
+added QmU5kp3BH3B8tnWUU2Pikdb2maksBNkb92FHRr56hyghh4 venus.txt
not ok 43 - 'ipfs add -r' output looks good
#
# PLANETS="QmWSgS32xQEcXMeqd3YPJLrNBLSdsfYCep2U7CFkyrjXwY" &&
# MARS="QmPrrHqJzto9m7SyiRzarwkqPcCSsKR2EB1AyqJfe8L8tN" &&
# VENUS="QmU5kp3BH3B8tnWUU2Pikdb2maksBNkb92FHRr56hyghh4" &&
# echo "added $MARS planets/mars.txt" >expected &&
# echo "added $VENUS planets/venus.txt" >>expected &&
# echo "added $PLANETS planets" >>expected &&
# test_cmp expected actual
#
Here is a quick patch to try, it might not apply cleanly for you though.
diff --git a/commands/files/readerfile.go b/commands/files/readerfile.go
index 2508fe1..d7f5443 100644
--- a/commands/files/readerfile.go
+++ b/commands/files/readerfile.go
@@ -4,6 +4,7 @@ import (
"errors"
"io"
"os"
+ gopath "path"
)
// ReaderFile is a implementation of File created from an `io.Reader`.
@@ -17,7 +18,11 @@ type ReaderFile struct {
baseInfo ExtraInfo
}
-func NewReaderFile(filename, path string, reader io.ReadCloser, stat os.FileInfo) *ReaderFile {
+func NewReaderFile(_, path string, reader io.ReadCloser, stat os.FileInfo) *ReaderFile {
+ filename := path
+ if path != "" {
+ filename = gopath.Base(path)
+ }
return &ReaderFile{filename, path, reader, stat, 0, PosInfo{0, path}}
}
@kevina Yeah, theres a LOT here, so its going to take some time. I don't disagree with you maintaining a fork in the meantime. That said, as I find time my plan is to pick out parts of the code that can be merged up easily. I think the next candidate for that is the commands/files code refactor to use absolute paths. |
e8c4a1b
to
2d09e9f
Compare
@whyrusleeping, One idea I had which might avoid some special casing is to replicate what is done with
That is create a special "node" for adding files to the filestore. This could help avoid special case code in at least a few places. It will at minimal avoid the special case code in the blockstore package as I can use my own implementation. Do you think this is an idea worth pursuing? I image the special node will share many of the data structures from the real node to avoid any data race or related issues. |
@kevina I think that might be a good idea, my only concern is that in order to read the blocks out of the filestore would you still need that custom created node? |
@whyrusleeping, no the custom node will be for adding only. We will still need some sort of multi-datastore for reading blocks. See my comments for #2747. |
@whyrusleeping there is a lot going on in an Ipfs Node. What I need is not a mock node like the code that With that in mind, I refactored the adder code a little to bundle all the needed services into the I use this new interface to to create a DataServices view in commit e6bfbfa that has uses an alternative Blockstore and leaves the existing Blockstore and caching Blockstore wrapper alone. In commit b54f2f4 I factor out the filestore specific code from the DagServices and instead install small hooks. In commit f9a51b1 I completely eliminated the AdvReader code you didn't like and instead computed the offset's in the DAG builder code (the Balanced builder to be specific, the Trickle builder doesn't support the filestore just yet). All in all I cleaned up a lot of the special case code. I you are okay with the changes in c41a47e I can separate that out into it's own pull request. |
@kevina from irc, we can move towards making the |
@whyrusleeping f5fb3c9 basically does what you ask for. |
None of the other methods in the measure package return this error, instead they only call RecordValue() when the value is []byte. This change makes batch Put consistent with the other methods and allows non []byte data to be passed though the measure datastore. License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
The datastore has an optional "advanced" datastore that handles Put requests for non []byte values, a "normal" datastore that handles all other put requests, and then any number of other datastore, some of them that can be designated read-only. Delete requests are passed on to all datastore not designed read-only. For now, querying will only work on a "normal" datastore. Note: Only tested in the case of just a "normal" datastore and the case of an "advanced" and "normal" datastore. License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
Create a new AdvReader interface that returns an ExtraInfo object that contains extra information about the the filehandle. This includes the current offset and the absolute path of a file. Also modify chunk.Splitter to return a Bytes struct in the NextBytes() method that in addition to the raw bytes returns the ExtraInfo object from the AdvReader that contains the current offset in the file (amoung other information). License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
…port. License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
Some Filestore clean operations are broken. Needs more tests. License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
Add --full-key option to "filestore ls" command. Add "ls" format option to "filestore verify". License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
Also fix bug discovered in "verify". License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
This reverts commit 241d10d. License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
This sure is a feat, pulled singlehandedly on filestore, 👏 👏, @kevina! If this PR is carefully splitted into 10 PR's consisting of 10 commits each (if 50% have already been merged), this should be doable within a week (or two the longest). |
🙌 🎆 #3629 has been merged, basic filestore functionality is in master. Go try it out :) |
Closes Issue #875, avoid duplicating files added to ipfs
NOT READY FOR MERGE
Rebased #2600 on master.
Quicklinks: Code, README,
TODO to get this merged:
Add DAGService.GetLinks() method and use it in the GC and elsewhere. #3255Note The filestore is very basic right now, but it is functional. I will likely continue to improve and submit new pull requests for the enhanced functionally but right now I fell it is important a basic implementation gets in so that it will get used, it can be labeled as an experimental feature and disabled by default, but available for those that want to use it. I consider the code production ready.
Resolves #875