Store sharding function used in the repo. #13

kevina · 2016-12-22T08:32:43Z

Store the sharding function used in the file "SHARDING" in the repo.
To make this work the sharding function is now always specified as a
string.

whyrusleeping · 2017-01-04T22:12:08Z

What are some example strings? We should make sure that these are somewhat nice multicodecs, cc @Kubuxu

whyrusleeping

This is looking good, a few things i noticed

whyrusleeping · 2017-01-04T20:05:44Z

flatfs.go

-		path:   path,
-		getDir: getDir,
-		sync:   sync,
+func New(path string, fun0 string, sync bool) (*Datastore, error) {


Lets have a New that detects the type from disk, and a NewWithFunc that has this signature

Okay, but I will still need the "auto" test case as that is used by the conversion utility.

I didn't do this yet. Do you basically want:

func New(path string, sync bool) (*Datastore, error) { NewWithFunc(path, "auto", sync) } func NewWithFunc(path string, fun0 string, sync bool) {...}

Is it that you don't like the special "auto" value? (I don't want to eliminate the use of that string as it makes writing the conversion function/utility easier, so using two separate functions won't really simplify any code.)

whyrusleeping · 2017-01-04T20:06:12Z

flatfs.go

-		str := padding + noslash
-		return str[len(str)-suffixLen:]
+	fun := ""
+	if fun0 == "auto" && fun1 == "auto" {


should make this a switch statement to be more idiomatic go

Okay. I don't particular see the difference, but don't object either. In the future I will try to remember that switch is generally preferred over multiple else ifs.

Kubuxu · 2017-01-06T11:14:30Z

What is the content of the SHARDING file directly? It isn't clear from the code?

kevina · 2017-01-06T14:02:12Z

What is the content of the SHARDING file directly? It isn't clear from the code?

Example file:

v1/next-to-last/2

As discussed in ipfs/kubo#3463

Kubuxu · 2017-01-06T14:08:25Z

Could you change it to previously proposed: /repo/flatfs/shard/v1/next-to-last/2, with leading slash. There are no costs in it but it might make someone's day some time in future.

kevina · 2017-01-06T14:19:00Z

@whyrusleeping agree with @Kubuxu? I have no problem doing it.

whyrusleeping · 2017-01-06T14:39:21Z

Yeap, agreed with @Kubuxu

kevina · 2017-01-07T18:02:12Z

Could you change it to previously proposed: /repo/flatfs/shard/v1/next-to-last/2

Done. Also refactored the parsing of the shard identifier string to make it more obvious what it going on.

whyrusleeping · 2017-01-09T20:26:35Z

flatfs.go

+		return nil, fmt.Errorf("shard function not specified")
+	case fun0 == "auto":
+		fun = fun1
+	case fun1 == "auto":


I think we should separate the task of opening an existing datastore and creating a new one. That way we can remove a lot of the weirdness here

I would rather not. The "weirdness" is also there to allow opening an existing repo without the SHARDING file.

mmm.... We're going to have to run a migration anyways. I think thats acceptable.

@whyrusleeping, okay to do things the way you want I will basically have to redo this pull request and the conversion code will require some adjusting. Let me know. (I'm only asking because you indicated you might do it in a private conversation and don't want to duplicate work.)

I started work on this. Will have it ready some time Wed.

kevina · 2017-01-11T05:35:12Z

@whyrusleeping okay please have another look, there are two fixme, but I think this is close to what you want. There is a separate Create and Open method.

This fix also makes the flatfs a little more robust, espacally after the fixme to check if an existing directory without a SHARDING file is not empty.

whyrusleeping

Githubs review thing is really annoying me... I thought I submitted this a day ago...

whyrusleeping · 2017-01-10T13:26:16Z

shard.go

+func parseShardFunc(str string) shardId {
+	str = strings.TrimSpace(str)
+	parts := strings.Split(str, "/")
+	// ignore prefix for now


we should check to make sure that the string has the expected prefix

whyrusleeping · 2017-01-10T13:27:27Z

shard.go

+	}
+}
+
+func parseShardFunc(str string) shardId {


we should a pointer to the shardId and an error here

whyrusleeping · 2017-01-10T13:27:39Z

shard.go

+		return shardId{funName: parts[0], param: parts[1]}
+	case 1:
+		return shardId{funName: parts[0]}
+	default: // can only happen for len == 0


this case should be an error

whyrusleeping · 2017-01-10T13:28:33Z

shard.go

+	}
+	switch len(parts) {
+	case 3:
+		return shardId{version: parts[0], funName: parts[1], param: parts[2]}


instead of returning strings here, we should probably do the parsing from Func() in here. That way we don't end up creating an 'invalid' shardId object.

Okay, I will see what I can do. I originally had parseShardFunc return an error but decided to push all the error checking into Func(). Will try refactoring again and see if I can make it work better the way I think you want.

whyrusleeping · 2017-01-10T13:31:57Z

shard.go

+}
+
+func ReadShardFunc(dir string) (string, error) {
+	fun := "auto"


We shouldnt ever return 'auto' from reading the func from disk. Either its a valid function, or its an error, try to validate input as much as possible as soon as possible.

whyrusleeping · 2017-01-10T13:34:20Z

shard.go

+	if err != nil {
+		return err
+	}
+	if str == IPFS_DEF_SHARD {


Lets separate the step of writing the readme from writing the sharding function, and also make sure to error out or print a log message if we try to write a readme for an unsupported sharding type

@whyrusleeping I don't think returning an error is a good idea. Especially if the flatfs datastore could be used outside of IPFS. I also don't like the idea of a warning for the same reason, but can add one if you still think it is a good idea.

I will separate the step from the writing of the sharding function.

whyrusleeping · 2017-01-10T13:43:50Z

flatfs_test.go

@@ -214,40 +218,42 @@ func testStorage(p *params, t *testing.T) {
 	if !seen {
 		t.Error("did not see the data file")
 	}
+	if fs.ShardFunc() == flatfs.IPFS_DEF_SHARD && !haveREADME {
+		t.Error("expected _README file")
+	} else if fs.ShardFunc() == flatfs.IPFS_DEF_SHARD && !haveREADME {


Are these two cases the same?

Oops. Yes. Fixed.

kevina · 2017-01-11T17:31:49Z

@whyrusleeping I should have addressed all your concerns in the latest review except for returning an error if no README can be created because I am not sure that it was a good idea. See my comment.

kevina · 2017-01-11T17:34:29Z

Note the two special files are named:

  _README
  SHARDING

I use '_README' because that is what @jbenet wanted. For consistency I can also rename 'SHARDING' to '_SHARDING' if you want. Let me know. For the record, I think using the special name '_README' is a good idea to help make it clear there is something special about it.

Kubuxu · 2017-01-13T18:14:26Z

flatfs.go

-		getDir: getDir,
-		sync:   sync,
+var DatastoreExists = errors.New("Datastore already exist")
+var DatastoreDoesNotExist = errors.New("Datastore directory does not exist")


Please use one var block and Err name prefix.

Kubuxu · 2017-01-13T18:18:23Z

shard.go

+		str = str[len(PREFIX):]
+	}
+	parts := strings.Split(str, "/")
+	if len(parts) == 3 {


Is the v1 optional? it doesn't have to be. Let's make it simpler.

I don't agree (with "it doesn't have to be") but @whyrusleeping would probably say the same thing so I will change it.

Kubuxu · 2017-01-13T18:19:07Z

shard.go

+}
+
+func ReadShardFunc(dir string) (*ShardIdV1, error) {
+	buf, err := ioutil.ReadFile(filepath.Join(dir, "SHARDING"))


Let's make the name of the file constant as it is reused.

Do we have a policy on this? In this particular case it seams like an overkill to me. We also don't seam to be very consistent about it elsewhere.

We try to use constants for these sorts of things when we can, There are definitely cases where we forget to do so, but in general its a good idea

Fine, Done. However, I think it makes code harder to read and I also am horrible at naming the things.

Kubuxu · 2017-01-13T18:22:35Z

flatfs.go

+		return nil, err
+	}
+
+	if err != nil {


Double err handler.

Oops. Will fix.

Kubuxu · 2017-01-13T18:24:41Z

shard.go

+			return nil, fmt.Errorf("invalid prefix in shard identifier: %s", str)
+		}
+		str = str[len(PREFIX):]
+	}


Let's use TrimPrefix, and check if output string is different than the input, if it is then prefix was trimmed.

Kubuxu · 2017-01-13T18:28:28Z

shard.go

+}
+
+func Prefix(prefixLen int) ShardFunc {
+	padding := strings.Repeat("_", prefixLen)


It there a better way to do it? It seems clever but it might have performance penalty (string concat), and it is done for every key, potentially multiple time.

This applies to other ShardFuncs too.

There might be yes. But this code has not changed, just moved. The same thing was also done before any of my shading changes. So for now (at least this p.r.) I would rather leave it alone.

[Sorry if this is a duplicate. GitHub is being annoying.]

kevina · 2017-01-13T21:47:51Z

@Kubuxu @whyrusleeping I should have addressed all your concerns. I also implemented the fixme.

The only outstanding issue is returning an error if no README can be created because I am not sure that it was a good idea. From my comment above:

Lets separate the step of writing the readme from writing the sharding function, and also make sure to error out or print a log message if we try to write a readme for an unsupported sharding type

@whyrusleeping I don't think returning an error is a good idea. Especially if the flatfs datastore could be used outside of IPFS. I also don't like the idea of a warning for the same reason, but can add one if you still think it is a good idea.

Kubuxu · 2017-01-13T21:48:09Z

shard.go

+
+func WriteReadme(dir string, id *ShardIdV1) error {
+	if id.String() == IPFS_DEF_SHARD {
+		err := ioutil.WriteFile(filepath.Join(dir, README_FN), []byte(IPFS_DEF_SHARD), 0444)


It should be README_DEF_SHARD

Okay. Told you I was bad a naming things. :)

Oops. Misunderstood and didn't see my own error. Will fix.

Kubuxu · 2017-01-13T21:48:59Z

shard.go

+	if err != nil {
+		return err
+	}
+	return nil


All above can be just return err.

Kubuxu · 2017-01-13T21:51:10Z

flatfs.go

+		if err != nil {
+			return err
+		}
+		return nil


can be just return err too (no need for if).

Kubuxu · 2017-01-13T21:51:48Z

flatfs.go

+
+	err := os.Mkdir(path, 0777)
+	if err != nil && !os.IsExist(err) {
+		return nil


Shouldn't it be return err?

kevina · 2017-01-13T22:30:52Z

@Kubuxu anything else :)

Store the sharding function used in the file "SHARDING" in the repo. To make this work the sharding function is now always specified as a string.

This avoids the need for the special "auto" string for the shard func.

Also use *ShardIdV1 instead of string for the shard fun parameter to WriteShardFunc and WriteReadme.

Other minor cleanups.

kevina · 2017-01-16T04:37:17Z

Note: Just rebased to clean up the commits, nothing else has changed.

kevina · 2017-01-16T06:20:31Z

Note: I just pushed some more commits to avoid the use of strings wherever possible.

whyrusleeping

This looks good to me now, Thanks for all the review iteration @Kubuxu and @kevina :)

kevina added the status/in-progress In progress label Dec 22, 2016

kevina mentioned this pull request Dec 22, 2016

Default 5 digit prefix for flatfs is no longer sufficient for CidV1 ipfs/kubo#3463

Closed

kevina requested a review from whyrusleeping January 4, 2017 19:21

whyrusleeping requested changes Jan 4, 2017

View reviewed changes

kevina mentioned this pull request Jan 7, 2017

Provide utility to convert from one Sharding type to another. #9

Merged

whyrusleeping reviewed Jan 9, 2017

View reviewed changes

kevina force-pushed the kevina/SHARDING branch from 5a92831 to bf2369a Compare January 11, 2017 05:37

whyrusleeping requested changes Jan 11, 2017

View reviewed changes

kevina force-pushed the kevina/SHARDING branch from 8f329f3 to ee4b8f3 Compare January 11, 2017 17:04

whyrusleeping requested a review from Kubuxu January 13, 2017 17:51

Kubuxu requested changes Jan 13, 2017

View reviewed changes

kevina force-pushed the kevina/SHARDING branch from 00c1048 to f3396ad Compare January 13, 2017 22:19

kevina force-pushed the kevina/SHARDING branch from c221959 to 574569a Compare January 13, 2017 23:25

kevina added 4 commits January 15, 2017 23:22

Store sharding function used in the repo.

d1554dd

Store the sharding function used in the file "SHARDING" in the repo. To make this work the sharding function is now always specified as a string.

Add constant for default shard func used by IPFS.

e89d36b

Add _README file when shard func is "v1/next-to-last/2"

ef9568a

Tweak TestStorage tests.

ce77b1d

kevina added 10 commits January 15, 2017 23:22

Tweak generated _README.

8dba91b

Prefix shard function identifier with "/repo/flatfs/shard/".

30b5a0e

Use separate Create and Open methods

d7bea76

This avoids the need for the special "auto" string for the shard func.

Refactor parsing of shard identifier.

f82967d

Separate out writing Readme file into its own function.

b02d206

Also use *ShardIdV1 instead of string for the shard fun parameter to WriteShardFunc and WriteReadme.

Test that the correct prefix is used in shard identifier.

fbc4d44

Minor cleanups.

b49a06d

Always require the version string.

9492e35

Test if directory is empty before creating SHARDING file.

9d9fdda

Other minor cleanups.

Use constants for SHARDING and _README file names.

6db29c0

kevina force-pushed the kevina/SHARDING branch from 574569a to 6db29c0 Compare January 16, 2017 04:28

kevina added 3 commits January 16, 2017 00:59

Avoid using strings for shard func when creating the datastore.

ceebea7

Code cleanup: Move things around so they are in more logical locations.

a156e6f

Always require the prefix in shard identifier.

b938a28

whyrusleeping approved these changes Jan 17, 2017

View reviewed changes

whyrusleeping merged commit 9553971 into master Jan 17, 2017

whyrusleeping deleted the kevina/SHARDING branch January 17, 2017 20:37

whyrusleeping removed the status/in-progress In progress label Jan 17, 2017

Store sharding function used in the repo. #13

Store sharding function used in the repo. #13

Conversation

kevina commented Dec 22, 2016

whyrusleeping commented Jan 4, 2017

whyrusleeping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu commented Jan 6, 2017

kevina commented Jan 6, 2017

Kubuxu commented Jan 6, 2017 • edited Loading

kevina commented Jan 6, 2017 • edited Loading

whyrusleeping commented Jan 6, 2017

kevina commented Jan 7, 2017

Choose a reason for hiding this comment

kevina Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina commented Jan 11, 2017

whyrusleeping 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

kevina Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

kevina commented Jan 11, 2017

kevina commented Jan 11, 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

kevina commented Jan 13, 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

kevina commented Jan 13, 2017

kevina commented Jan 16, 2017

kevina commented Jan 16, 2017

whyrusleeping left a comment

Choose a reason for hiding this comment

kevina Jan 6, 2017 •

edited

Loading

Kubuxu commented Jan 6, 2017 •

edited

Loading

kevina commented Jan 6, 2017 •

edited

Loading

kevina Jan 9, 2017 •

edited

Loading

kevina Jan 9, 2017 •

edited

Loading

kevina Jan 11, 2017 •

edited

Loading