Basic Filestore Utilties #3653

kevina · 2017-02-04T01:48:05Z

No description provided.

kevina · 2017-02-04T01:48:29Z

Only filestore ls and filestore verify are implemented now. Still needs tests.

kevina · 2017-02-04T01:48:58Z

@whyrusleeping please let me know if you are okay with these changes so far.

whyrusleeping · 2017-02-06T17:56:02Z

core/commands/filestore.go

+		if res.Error() != nil {
+			return
+		}
+		outChan, ok := res.Output().(<-chan interface{})


Why is this in the PostRun. Shouldnt this be a marshaler?

This is in PostRun becuase I output to both stdout and stderr something that a marshaler can't do, unless I am mistaken.

Note, I did a very similar thing in ipfs block rm: https://github.com/ipfs/go-ipfs/blob/master/core/commands/block.go#L283.

Also ipfs add does not use a marshaler.

This should be able to go in the marshaler. You can write to stderr and stdout in the marshaler the same way you can in the PostRun. If we do weird things like this in the post run then it breaks the API expectations in a weird way.

@whyrusleeping so should the Marshalers just return nil for the Reader?

@whyrusleeping okay done, let me know if its okay now.

whyrusleeping · 2017-02-06T18:00:16Z

filestore/util.go

+	}
+
+	k := ds.RawKey(v.Key)
+	c, err := dshelp.DsKeyToCid(k)


shouldnt this reuse the existing method for reading a dataobj ?

what method is that?

this one? https://github.com/ipfs/go-ipfs/blob/feat/filestore0/filestore/fsrefstore.go#L100

Reusing that method will involve an extra lookup as we already have the value from the iterator.

Then refactor that method so that it looks up the value and calls some dataObjFromValue method

Okay, will look into it tomorrow.

whyrusleeping · 2017-02-06T18:30:12Z

filestore/util.go

+		} else if err, ok := err.(*CorruptReferenceError); ok {
+			switch err.Code {
+			case FileError:
+				status = StatusFileError


Why not just reuse the codes from the CorruptReferenceError ?

The set of codes returned in ListRes is larger than the codes for CorruptReferenceError. For a little less type safety I can have CorruptReferenceError use the same codes in ListRes if you prefer it that way,

Yeah, lets unify the sets of codes

whyrusleeping · 2017-02-06T18:31:10Z

Also make sure to clean out commented out imports

kevina · 2017-02-06T18:36:47Z

Also make sure to clean out commented out imports

Will do towards the end, some of them might still be needed.

kevina · 2017-02-08T04:18:47Z

@whyrusleeping this is about done, the final command I plan to add is "clean" which will remove invalid blocks. I will get to that early wed.

whyrusleeping · 2017-02-09T00:53:41Z

core/commands/filestore.go

+				}
+			}
+		}()
+		res.SetOutput(reader)


Don't use a reader for output like this. Send objects over a channel like we do in the other commands so that the api is easier to deal with.

Okay, all I am sending is Cid's, is there a Marshaler I can reuse?

The one from ipfs refs should work

whyrusleeping · 2017-02-09T01:09:23Z

filestore/util.go

+func (r *ListRes) FormatLong() string {
+	switch {
+	case r.Key == nil:
+		return "?????????????????????????????????????????????????"


If this is never expected to happen, we should probably panic here instead of printing an ugly string

It might happen, if the key is corrupt, in which case there will also be an error. The idea is to not abort and be able to move on to the next entry.

Then maybe print something useful like "corrupted key", or maybe error this entry out earlier when its detected the key is bad.

This is what a listing with a corrupt key will likely look like to stdout:

zb2rhaPkR7ZF9BzSC2BfqbcGivi9QMdauermW9YB6NvS7FZMo 10000 somedir/file2 0 zb2rhav4wcdvNXtaKDTWHYAqtUHMEpygT1cxqMsfK7QrDuHxH 262144 somedir/file3 524288 zb2rhbcZ3aUXYcrbhhDH1JyrpDcpdw1KFJ5Xs5covjnvMpxDR 1000 somedir/file1 0 ????????????????????????????????????????????????? zb2rhebtyTTuHKyTbJPnkDUSruU5Uma4DN8t2EkvYZ6fP36mm 262144 somedir/file3 262144 zb2rhm9VTrX2mfatggYUk8mHLz78XBxVUTTzLvM2N3d6frdAU 213568 somedir/file3 786432

And to stderr:

error: could not decode cid: some-bad-key-string

The key may be corrupt, or any number of other things. I will be happy to replace ????????????????????????????????????????????????? with whatever you want (as long as it can not also be a valid key) but I don't want to panic as that will create an unnecessary special case.

maybe <corrupt key string> ?

I prefer ????????????????????????????????????????????????? but okay, I'll change it tomorrow.

whyrusleeping · 2017-02-09T01:11:19Z

filestore/util.go

+	}
+
+	k := ds.RawKey(v.Key)
+	c, err := dshelp.DsKeyToCid(k)


this one? https://github.com/ipfs/go-ipfs/blob/feat/filestore0/filestore/fsrefstore.go#L100

whyrusleeping · 2017-02-09T01:33:31Z

Also make sure to clean up the unused imports

whyrusleeping · 2017-02-09T01:36:39Z

test/sharness/t0271-filestore-utils.sh

+	'
+
+	test_expect_success "'ipfs filestore ls' output looks good'" '
+		ipfs filestore ls | LC_ALL=C sort > ls_actual &&


What is the LC_ALL=C for? That seems really weird

The LC_ALL is to avoid any influence from the locale environment in the sort order.

It isn't needed, sharness sets it.

whyrusleeping · 2017-02-09T01:38:09Z

test/sharness/t0271-filestore-utils.sh

+'
+
+	test_expect_success "block okay now" '
+		ipfs cat $FILE1_HASH > /dev/null


Probably want to shasum the output or something just to be sure. If the filestore read verification was broken then this test could pass.

Also, formatting. The closing quotes should line up with the test_expect_success

Probably want to shasum the output or something just to be sure. If the filestore read verification was broken then this test could pass.

okay

Also, formatting.

Sorry. The formatting is done manually. Will double check.

mib-kd743naq · 2017-02-09T10:13:13Z

I prefer ????????????????????????????????????????????????? ...

@kevina perhaps I am pointing the obvious, but visual alignment would not possible anyway in more complex scenarios where the hashing type / length is not uniform. E.g. cid links to pb content and the like

kevina · 2017-02-09T16:40:31Z

perhaps I am pointing the obvious, but visual alignment would not possible anyway in more complex scenarios

@mib-kd743naq yeah I know. I prefer the ???... because it is non specific, just that there is a problem. It's not a big deal at all.

Kubuxu

Some comments, I will probably have more when I read it once more.

Kubuxu · 2017-02-12T19:51:53Z

test/sharness/t0271-filestore-utils.sh

+
+echo "WORKING DIR"
+echo "IPFS PATH = " $IPFS_PATH
+pwd


Debug code?

Ask @whyrusleeping I was using t0270-filestore-utils.sh as a template.

Kubuxu · 2017-02-12T19:53:37Z

test/sharness/t0271-filestore-utils.sh

+
+cat <<EOF > dups_expect
+$FILE1_HASH
+EOF


Is it outside of the test?

Yes, I didn't think it mattered. I can move it into a test if it's important.

Kubuxu · 2017-02-12T19:54:34Z

test/sharness/t0271-filestore-utils.sh

+echo "IPFS PATH = " $IPFS_PATH
+pwd
+
+


General comment here, you seem to interleave workspace prep, function definitions and constant definitions, could we clean it up?

Yeah I know. I didn't think it really matters. I will see what I can do.

It does as it makes harder to understand the tests, which means that in future, when they will have to be changed, already high chance of mistake will be even higher.

Kubuxu · 2017-02-12T19:58:07Z

core/commands/filestore.go

+			return
+		}
+
+		out := make(chan interface{})


Probably best to add some buffer.

Kubuxu · 2017-02-12T19:59:18Z

core/commands/filestore.go

+`,
+	},
+	Arguments: []cmds.Argument{
+		cmds.StringArg("hash", true, true, "Bash58 encoded multihash of block(s) to remove."),


not bash but Base58, but as it is CID then it should accept just CID. CID can have base descriptor.

Oops, will fix.

Kubuxu · 2017-02-12T20:00:21Z

core/commands/root.go

@@ -124,6 +124,7 @@ var rootSubcommands = map[string]*cmds.Command{
 	"update":    ExternalBinary(),
 	"version":   VersionCmd,
 	"bitswap":   BitswapCmd,
+	"filestore": FileStoreCmd,


It should be mentioned in helptext of main command.

Oversight. Will fix.

Kubuxu · 2017-02-12T20:08:48Z

Can you also rebase it to master.

kevina · 2017-02-12T20:10:48Z

Can you also rebase it to master.

No, it depends on feat/filestore0.

Kubuxu · 2017-02-12T20:12:04Z

Can I rebase feat/filestore0 to master?

whyrusleeping · 2017-03-07T07:58:28Z

@kevina cool, will review

whyrusleeping · 2017-03-11T01:13:22Z

Slating for 0.4.8, will review in the next few days

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

…ng file. License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

…mands. License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

kevina · 2017-03-22T20:25:38Z

Rebased yet again...

whyrusleeping · 2017-03-22T23:16:57Z

core/commands/filestore.go

+
+var dupsFileStore = &cmds.Command{
+	Helptext: cmds.HelpText{
+		Tagline: "Print block both in filestore and non-filestore.",


Change to "List blocks that are both in the filestore and standard block storage."

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

kevina · 2017-03-23T01:01:38Z

Factored out filestore rm as requested on IRC.

whyrusleeping · 2017-03-23T04:41:35Z

test failures are unrelated (though still concerning). I've noted both of them and will be investigating soon.

Otherwise, lets get this show on the road! 🚢

kevina added the status/in-progress In progress label Feb 4, 2017

whyrusleeping force-pushed the feat/filestore0 branch 2 times, most recently from 2b3bd24 to d9c40b5 Compare February 6, 2017 00:40

kevina force-pushed the kevina/filestore-util branch from 6371a0d to b8343e0 Compare February 6, 2017 18:21

whyrusleeping requested changes Feb 6, 2017

View reviewed changes

whyrusleeping reviewed Feb 6, 2017

View reviewed changes

kevina force-pushed the kevina/filestore-util branch from 7b01409 to 2340263 Compare February 7, 2017 02:54

whyrusleeping reviewed Feb 9, 2017

View reviewed changes

whyrusleeping requested changes Feb 9, 2017

View reviewed changes

whyrusleeping reviewed Feb 9, 2017

View reviewed changes

whyrusleeping requested a review from Kubuxu February 9, 2017 03:41

whyrusleeping added this to the ipfs 0.4.6 milestone Feb 12, 2017

Kubuxu requested changes Feb 12, 2017

View reviewed changes

whyrusleeping force-pushed the feat/filestore0 branch from d9c40b5 to 299f5c9 Compare February 12, 2017 20:52

kevina force-pushed the kevina/filestore-util branch from c3d6cd6 to 10e2401 Compare February 12, 2017 21:20

whyrusleeping modified the milestones: Ipfs 0.4.7, ipfs 0.4.6 Feb 15, 2017

kevina mentioned this pull request Feb 20, 2017

block rm: use Marshalers instead of PostRun to process output #3708

Merged

whyrusleeping force-pushed the feat/filestore0 branch from 299f5c9 to 1a94991 Compare March 2, 2017 00:49

whyrusleeping self-assigned this Mar 7, 2017

whyrusleeping self-requested a review March 11, 2017 01:13

whyrusleeping modified the milestones: Ipfs 0.4.8, Ipfs 0.4.7 Mar 11, 2017

kevina added 7 commits March 22, 2017 16:24

filestore util: basic filestore commands.

4bf4304

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

filestore util: allow listing/verifying of individual blocks.

13f617d

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

filestore: be more specific when there is a problem reading the backi…

886a6fe

…ng file. License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

filestore util: add documentation for 'filestore ls' and 'verify' com…

064bb53

…mands. License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

filestore util: add tests for verifying and listing filestore blocks.

033c442

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

filestore: use the same codes in ListRes and CorruptReferenceError.

1f2e174

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

filestore util: Add 'filestore dups' command. Enhance tests.

f7efb34

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

kevina force-pushed the kevina/filestore-util branch from c40d42d to 77648f0 Compare March 22, 2017 20:25

whyrusleeping reviewed Mar 22, 2017

View reviewed changes

kevina added 3 commits March 22, 2017 19:55

filestore util: change "???..." to "<corrupt key>"

81af034

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

filestore: Refactor.

96269e0

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

filestore util: refactor and clean up tests

620b52b

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

kevina force-pushed the kevina/filestore-util branch from 77648f0 to 620b52b Compare March 23, 2017 00:54

filestore util: doc improvement

1eddb60

License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

kevina mentioned this pull request Mar 23, 2017

filestore rm #3815

Open

whyrusleeping approved these changes Mar 23, 2017

View reviewed changes

whyrusleeping merged commit 2de21bc into master Mar 23, 2017

whyrusleeping removed status/in-progress In progress labels Mar 23, 2017

kevina mentioned this pull request Mar 23, 2017

filestore util: Use a Marshaler instead of PostRun... #3820

Merged

Kubuxu deleted the kevina/filestore-util branch August 10, 2017 17:10

Basic Filestore Utilties #3653

Basic Filestore Utilties #3653

Conversation

kevina commented Feb 4, 2017

kevina commented Feb 4, 2017

kevina commented Feb 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Feb 6, 2017

kevina commented Feb 6, 2017

kevina commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mib-kd743naq commented Feb 9, 2017

kevina commented Feb 9, 2017

Kubuxu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu commented Feb 12, 2017

kevina commented Feb 12, 2017

Kubuxu commented Feb 12, 2017

whyrusleeping commented Mar 7, 2017

whyrusleeping commented Mar 11, 2017

kevina commented Mar 22, 2017

Choose a reason for hiding this comment

kevina commented Mar 23, 2017

whyrusleeping commented Mar 23, 2017

kevina Feb 9, 2017 •

edited

Loading