Various refactorings #36

schomatis · 2018-12-16T18:01:45Z

This PR contains a series of commits with diverse MFS code refactorings. The commits are loosely tied together here to make it easier to review and merge. This PR then should be reviewed on a commit-by-commit basis in chronological order.

This PR breaks the current API (which is something expected in the context of the milestone to review the MFS API) but only in evident ways, e.g., renaming/removing functions, it doesn't change the current behavior (those kind of changes will be discussed in separate issues/PRs).

Unify the `string`/`ipld.Node` information a parent has about its children when updating a modified entry. This will be used to simplify the code around the `childCloser` interface and related logic.

Use newly introduced `child` structure whenever the logically tied `string`/`ipld.Node` pair was used to represent a child entry in a directory operation.

Rename the `childCloser` interface to `mutableParent` to better reflect the fact that the structures that implement it (`Directory` and `Root`) have the function of parents in the MFS hierarchy. Rename also the method of the interface from `closeChild` to `updateChildEntry` shifting the focus away from in which circumstance is the method being called (when closing a child) to actually what operation is being performed by the method (updating an entry in the parent to the new content).

Extract the `Republisher` structure and related logic from `system.go` (leaving that file mainly with the `Root` logic, and hence now renamed as `root.go`) to a new `repub.go` file. Even though the MFS root has support for republishing updates the logic behind those two structures is mostly decoupled, that should be reflected in the source code organization.

Remove the `Sync()` method from the `FileDescriptor` and also its implementation. Two different undocumented methods in `FileDescriptor` are offered to update the state of the MFS: `Flush()` and `Sync()`, both calling `updateChildEntry`. The only difference (the user is not aware of) is that one sets the `fullSync` argument and the other doesn't, that is, one does a full update of the filesystem and the other just updates the parent directory. This current situation is not clear to the consumer who may use one desiring the effect of the other. As a precautionary measure while redesigning the MFS API one method is removed to simplify its usage. The `Sync()` was chosen as the less "safe" alternative which didn't do a full update of the MFS and also doesn't seem to be used in the `go-ipfs` repository (the main consumer of the `mfs` package).

Unify the two caches in `Directory` that discriminated between files and directories (`files` and `childDirs`) into a single `entriesCache` since that distinction wasn't used anywhere and made the code unnecessarily more complex.

schomatis · 2018-12-17T23:25:55Z

/cc @Stebalien

schomatis · 2018-12-18T21:27:38Z

@Stebalien ping :)

Stebalien · 2018-12-19T00:07:00Z

dir.go

@@ -405,7 +405,7 @@ func (d *Directory) AddChild(name string, nd ipld.Node) error {

 // AddUnixFSChild adds a child to the inner UnixFS directory
 // and transitions to a HAMT implementation if needed.
-func (d *Directory) AddUnixFSChild(name string, node ipld.Node) error {
+func (d *Directory) AddUnixFSChild(c child) error {


Public functions shouldn't take private types.

Stebalien · 2018-12-19T00:10:40Z

system.go

 // TODO: What is `fullsync`? (unnamed `bool` argument)
 // TODO: There are two types of persistence/flush that need to be
 // distinguished here, one at the DAG level (when I store the modified
 // nodes in the DAG service) and one in the UnixFS/MFS level (when I modify
 // the entry/link of the directory that pointed to the modified node).
-type childCloser interface {
-	closeChild(child, bool) error
+type mutableParent interface {


Why not just parentHandle, or even just parent? I agree that childCloser is not the right name but mutableParent signals "this is a mutable parent, there are immutable versions".

system.go

Stebalien · 2018-12-19T00:17:13Z

dir.go

+// So, `fullSync` entails operations at two different layers:
+//   1. DAG: save the newly created directory node with the updated entry.
+//   2. MFS: propagate the update upwards repeating the whole process in the
+//           parent.


Let's be concise and to the point.

fd.go

Stebalien · 2018-12-19T00:54:05Z

dir.go

@@ -237,12 +237,6 @@ func (d *Directory) Child(name string) (FSNode, error) {
 	return d.childUnsync(name)
 }

-func (d *Directory) Uncache(name string) {


This is important. We use it from the unixfs file adder in go-ipfs to avoid a massive memory leak.

This isn't ideal but we can't remove it without replacement. It was added to fix a critical bug.

Document the `Republisher` code in `repub.go` according to the explanation in ipfs/kubo#5092 (comment). Erring on the side of verbosity since this has been a part of the code with very little review up until now.

Unify all the `Republisher` receivers names to `rp` (avoiding `r` to distinguish it from the `Root` receivers).

schomatis · 2018-12-19T15:39:27Z

@Stebalien Addressed your comments in the added (last) 3 commits and dropped the one that removed the Uncache method (I'll follow up on that in ipfs/kubo#3027), could you take another look please to check if this is ready to go?

Stebalien · 2018-12-19T17:21:02Z

dir.go

@@ -405,7 +403,7 @@ func (d *Directory) AddChild(name string, nd ipld.Node) error {

 // AddUnixFSChild adds a child to the inner UnixFS directory
 // and transitions to a HAMT implementation if needed.
-func (d *Directory) AddUnixFSChild(name string, node ipld.Node) error {
+func (d *Directory) AddUnixFSChild(c Child) error {


As an internal helper type, child is fine (although I still feel it's unecessary). However, I really don't want to see it leak into an external interface. All it does is change calls to:

d.AddUnixFSChild(name, node)

To:

d.AddUnxiFSChild(mfs.Child{Name: name, Node: node})

Which is strictly more annoying to use and provides no additional safety, documentation, etc.

(sorry, I should have commented on this the first round but I assumed you'd replace child with the name/node in this case instead of exporting it)

Thanks for pointing me to it, actually AddUnixFSChild shouldn't be exported in the first place, I'll also un-export child.

The name `mutableParent` signals "this is a mutable parent, there are immutable versions".

schomatis · 2018-12-19T17:43:04Z

@Stebalien Fixed.

Stebalien

LGTM.

schomatis · 2018-12-19T17:53:19Z

@Stebalien Thanks for helping me get this PR in shape!

Stebalien · 2018-12-19T18:01:06Z

Thanks for cleaning all this up!

schomatis added status/in-progress In progress topic/technical debt Topic technical debt labels Dec 16, 2018

schomatis added this to the Review the MFS interface milestone Dec 16, 2018

schomatis self-assigned this Dec 16, 2018

schomatis mentioned this pull request Dec 17, 2018

MFS Republisher ipfs/kubo#5092

Closed

schomatis added 7 commits December 17, 2018 01:15

add a child structure

32ca97a

Unify the `string`/`ipld.Node` information a parent has about its children when updating a modified entry. This will be used to simplify the code around the `childCloser` interface and related logic.

use new child structure

54062f4

Use newly introduced `child` structure whenever the logically tied `string`/`ipld.Node` pair was used to represent a child entry in a directory operation.

document code around Directory.updateChildEntry

ef5e119

unify the caches in Directory

925c0a8

Unify the two caches in `Directory` that discriminated between files and directories (`files` and `childDirs`) into a single `entriesCache` since that distinction wasn't used anywhere and made the code unnecessarily more complex.

schomatis force-pushed the feat/general/refactoring branch from 06cd484 to 9186d93 Compare December 17, 2018 04:23

schomatis changed the title ~~[WIP] Various refactorings~~ Various refactorings Dec 17, 2018

schomatis added needs review and removed status/in-progress In progress labels Dec 17, 2018

schomatis requested a review from Stebalien December 17, 2018 04:25

schomatis mentioned this pull request Dec 17, 2018

Avoid closeChild calls altogether if flush/sync is not set, remove our internal sync option #39

Closed

Stebalien reviewed Dec 19, 2018

View reviewed changes

schomatis mentioned this pull request Dec 19, 2018

don't cache entire mfs tree on add finalize ipfs/kubo#3027

Merged

ghost added the status/in-progress In progress label Dec 19, 2018

schomatis added 2 commits December 19, 2018 12:35

document republisher logic

0abe769

Document the `Republisher` code in `repub.go` according to the explanation in ipfs/kubo#5092 (comment). Erring on the side of verbosity since this has been a part of the code with very little review up until now.

rename all Republisher receivers

4809106

Unify all the `Republisher` receivers names to `rp` (avoiding `r` to distinguish it from the `Root` receivers).

schomatis force-pushed the feat/general/refactoring branch from 1930199 to cb36051 Compare December 19, 2018 15:35

Stebalien suggested changes Dec 19, 2018

View reviewed changes

schomatis added 2 commits December 19, 2018 14:38

rename the mutableParent interface to just parent

d1471da

The name `mutableParent` signals "this is a mutable parent, there are immutable versions".

reword updateChildEntry method documentation

25dd99b

unexport AddUnixFSChild

6c4a00a

schomatis force-pushed the feat/general/refactoring branch from cb36051 to 6c4a00a Compare December 19, 2018 17:42

Stebalien approved these changes Dec 19, 2018

View reviewed changes

schomatis merged commit e5a375d into master Dec 19, 2018

ghost removed the status/in-progress In progress label Dec 19, 2018

schomatis deleted the feat/general/refactoring branch December 19, 2018 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various refactorings #36

Various refactorings #36

schomatis commented Dec 16, 2018 •

edited

Loading

schomatis commented Dec 17, 2018

schomatis commented Dec 18, 2018

Stebalien Dec 19, 2018

Stebalien Dec 19, 2018

Stebalien Dec 19, 2018

Stebalien Dec 19, 2018

schomatis commented Dec 19, 2018

Stebalien Dec 19, 2018

schomatis Dec 19, 2018

schomatis commented Dec 19, 2018

Stebalien left a comment

schomatis commented Dec 19, 2018

Stebalien commented Dec 19, 2018

Various refactorings #36

Various refactorings #36

Conversation

schomatis commented Dec 16, 2018 • edited Loading

schomatis commented Dec 17, 2018

schomatis commented Dec 18, 2018

Stebalien Dec 19, 2018

Choose a reason for hiding this comment

Stebalien Dec 19, 2018

Choose a reason for hiding this comment

Stebalien Dec 19, 2018

Choose a reason for hiding this comment

Stebalien Dec 19, 2018

Choose a reason for hiding this comment

schomatis commented Dec 19, 2018

Stebalien Dec 19, 2018

Choose a reason for hiding this comment

schomatis Dec 19, 2018

Choose a reason for hiding this comment

schomatis commented Dec 19, 2018

Stebalien left a comment

Choose a reason for hiding this comment

schomatis commented Dec 19, 2018

Stebalien commented Dec 19, 2018

schomatis commented Dec 16, 2018 •

edited

Loading