Builder refactoring, trickle builder and balanced builder #118

pgte · 2016-12-28T17:12:03Z

Refactored the builder constructors and streams.
A builder now is a transform stream that reduces into one root. (some opportunities for externalising these reducers into their own packages).
Each builder can be parametrised by options.
Documented those options.
Client can also choose which builder to use (now defaults to balanced builder).
Adds individual tests for each reducer stream to make sure it's yielding the correct tree structure.

By the way,
Can now easily add a different chunker.
Client can choose chunker strategy using option. Defaults and only supports fixed chunker for now.

Please review.

dignifiedquire · 2016-12-29T13:41:13Z

README.md

+In the second argument of the importer constructor you can specify the following options:
+
+* `chunker` (string, defaults to `"fixed"`): the chunking strategy. Now only supports `"fixed"`
+* `chunkSize` (positive integer, defaults to `262144`): the maximum chunk size for the `fixed` chunker.


I think it would be better to have a specific chunkerOptions object, which has in the case for the fixed chunker an option of size, as for other chunkers this might not make sense as an option.

@dignifiedquire Implemented your suggestion.

dignifiedquire · 2016-12-29T14:03:30Z

src/builder/builder.js

+
+const defaultOptions = {
+  chunkSize: 262144
+  // chunkSize: 26214


What's up with this lonely commented out option?

Was used in an experiment and had forgotten to remove it. Removed now.

dignifiedquire · 2016-12-29T14:32:40Z

@pgte good stuff, one thing I'm a bit worried about, is the usage of pushable in a quite a lot of places. I haven't gone through all of them but I think some of them could be removed in favor of more direct transforms. The same goes for the use of pullWritable, as that forces you to use something like pushable if you want to use it in a transform stream.

I haven't found simple solutions for where you used these, so this might be the best way for these use cases, but maybe not.

@dominictarr maybe you have some good ideas about how the use of pushable could be avoided in these situations.

dominictarr · 2016-12-29T17:14:00Z

instead of using pull-write to write to pull-pushable,
i think what you can do here is use either pull-defer,
or pull-pair because what it looks like you are doing is creating a source stream that initially doesn't output anything, until later when you get the real source. They way you are doing it with pushable looses back pressure, but with pull-pair or pull-defer or maybe pull-cont then you can do the same thing but still have back pressure.

pgte · 2016-12-30T10:29:40Z

@dominictarr thanks, good point about the loss of back-pressure.
I'm now using pull-pair instead of the bespoke back-pressureless version.

daviddias

Solid! I've little or no comments codewise, big thumbs up for the refactor 👍

Couple of requests:

We need a couple of exporter tests for the new added importer strategies
We need a test to make sure that if several roots are passed to one importer instance, it errors out (because it only knows how to return one root). This is a test that was missing.

daviddias · 2016-12-30T10:45:43Z

README.md

+  * `trickle`: builds [a trickle tree](https://github.com/ipfs/specs/pull/57#issuecomment-265205384)
+* `maxChildrenPerNode` (positive integer, defaults to `172`): the maximum children per node for the `balanced` and `trickle` DAG builder strategies
+* `layerRepeat` (positive integer, defaults to 4): (only applicable to the `trickle` DAG builder strategy). The maximum repetition of parent nodes for each layer of the tree.
+


@dignifiedquire wanna push a commit with jsdoc to this PR so that unixfs-engine gets fresh docs with this refactor? Or perhaps just to a PR to the PR, that would be also good :)

We can merge this as is and we can add the documentation in a later PR, other wise if @pgte wants to take a stab he can start adding this info as jsdoc as well. It doesn't have to be me to do that.

dignifiedquire · 2016-12-30T12:31:17Z

src/builder/balanced/balanced-reducer.js

+
+  const result = pushable()
+
+  reduceToParents(source, function (err, roots) {


please use arrow functions for unnamed functions, to keep consistent with the rest of our code base

dignifiedquire · 2016-12-30T12:33:00Z

src/builder/builder.js

+  }
+}
+
+module.exports = function (Chunker, ipldResolver, Reducer, _options) {


Please use uppercase variables only for constructors, and if they are constructors call them with new.

dignifiedquire · 2016-12-30T12:34:03Z

src/builder/flat/index.js

+    source,
+    batch(Infinity),
+    pull.asyncMap(reduce),
+    pull.collect(function (err, roots) {


arrow function

dignifiedquire · 2016-12-30T12:37:57Z

test/test-balanced-builder.js

+}
+
+describe('balanced builder', () => {
+  it('reduces one value into itself', callback => {


please use parens around the args for arrow functions

dignifiedquire · 2016-12-30T12:38:03Z

test/test-balanced-builder.js

+    )
+  })
+
+  it('reduces 3 values into parent', callback => {


Let's add this to the aegir-lint

I always thought standard was linting for this, which is why I adopted it in the first place

On the other hand, given this standard/eslint-config-standard#54 (comment) gigantic history I don't feel like touching it tbh

pgte · 2017-01-03T14:05:54Z

@diasdavid both requested changes are done via:

pgte · 2017-01-04T09:27:53Z

@diasdavid there are also some opportunities to create more independent packages from this, namely the pull streams that generate a balanced tree and the trickle tree..

daviddias · 2017-01-09T09:47:33Z

README.md

@@ -163,7 +163,7 @@ In the second argument of the importer constructor you can specify the following
  * `trickle`: builds [a trickle tree](https://github.com/ipfs/specs/pull/57#issuecomment-265205384)
 * `maxChildrenPerNode` (positive integer, defaults to `172`): the maximum children per node for the `balanced` and `trickle` DAG builder strategies
 * `layerRepeat` (positive integer, defaults to 4): (only applicable to the `trickle` DAG builder strategy). The maximum repetition of parent nodes for each layer of the tree.
-
+* `reduceSingleLeafToSelf` (boolean, defaults to `false`): optimization for, when reducing a set of nodes with one node, reduce it to that node.


daviddias · 2017-01-09T09:48:12Z

src/builder/balanced/index.js

@@ -3,7 +3,7 @@
 const balancedReducer = require('./balanced-reducer')

 const defaultOptions = {
-  maxChildrenPerNode: 172
+  maxChildrenPerNode: 174
 }


Let's add a comment with a link to https://github.com/ipfs/go-ipfs/blob/master/importer/helpers/helpers.go#L16-L35 so that we remember where this value comes from

daviddias · 2017-01-09T09:51:11Z

README.md

+  * `flat`: flat list of chunks
+  * `balanced`: builds a balanced tree
+  * `trickle`: builds [a trickle tree](https://github.com/ipfs/specs/pull/57#issuecomment-265205384)
+* `maxChildrenPerNode` (positive integer, defaults to `172`): the maximum children per node for the `balanced` and `trickle` DAG builder strategies


Now defaults to 174

daviddias · 2017-01-11T13:03:26Z

🎉🎉🎉🎉🎉🎉🎉🎉

THIS IS HAPPENING :D

Thank you @pgte ❤️❤️❤️❤️❤️

pgte added 5 commits December 28, 2016 16:39

builder refactoring. trickle builder. balanced builder

511c746

removed unused experimental builder

0e3f158

documented importer options

6af7458

default builder strategy is now the balanced strategy

977f41b

removed unused test

c767848

RichardLitt added the status/in-progress In progress label Dec 28, 2016

pgte requested a review from daviddias December 28, 2016 17:18

pgte added 2 commits December 28, 2016 18:13

removed superfluous comment

7854682

fixed trickle builder

df48f69

dignifiedquire reviewed Dec 29, 2016

View reviewed changes

pgte added 4 commits December 29, 2016 14:35

removed superfluous comment

50c5d35

using options.chunkerOptions for chunker-specific options

cbe2ce4

docs: corrected option name

f8b9e80

fix: error handling in trickle reducer

fedfc30

using pull-pair instead of backpressure-less bespoke pair

8e8d3d6

daviddias suggested changes Dec 30, 2016

View reviewed changes

dignifiedquire reviewed Dec 30, 2016

View reviewed changes

pgte added 5 commits January 3, 2017 12:04

fixed trickle builder tests

7647657

recursive streaming trickle builder

74482f3

missing dep

2b92345

some style corrections

01d8583

importing multiple roots yields an error

0036314

pgte added 3 commits January 3, 2017 12:58

reinstated testing importing using flat and balanced strategies

02cdefd

asserting that root node is one and only one

8ac163c

testing import and export using various builder strategies

e723586

pgte added 2 commits January 3, 2017 19:29

fixed error propagation into push streams

b9a01f8

simplified some iteration logic

03f49d4

pgte added 3 commits January 7, 2017 22:01

default for maximum children pre node is 174

fedbe5f

by default, only reduces one leaf to self if specific option is present

180b808

test results reflect new default config

937c292

daviddias reviewed Jan 9, 2017

View reviewed changes

pgte added 2 commits January 9, 2017 15:53

testing against big files genearted from a pseudo random byte stream gen

0f706df

added missing dep

0d3602e

pgte changed the title ~~WIP: Builder refactoring, trickle builder and balanced builder~~ Builder refactoring, trickle builder and balanced builder Jan 9, 2017

This was referenced Jan 10, 2017

WIP: support the new unixfs-engine balanced builder ipfs-inactive/interface-js-ipfs-core#106

Closed

WIP: tests: new expected hashes and sizes for the new unixfs-engine defaul… ipfs/js-ipfs#705

Closed

pgte added 3 commits January 10, 2017 09:41

removed unnecessary dev dependency

973c483

go-ipfs parity: no root node with single leaf

67fbf87

docs: corrected the default maximum number of children nodes

ff6cce5

daviddias reviewed Jan 11, 2017

View reviewed changes

daviddias merged commit c1acd5b into master Jan 11, 2017

daviddias deleted the feat/builders branch January 11, 2017 13:00

daviddias removed the status/in-progress In progress label Jan 11, 2017

daviddias mentioned this pull request Jan 29, 2017

Support for UnixFS Sharding + support balanced & trickle dag #45

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Builder refactoring, trickle builder and balanced builder #118

Builder refactoring, trickle builder and balanced builder #118

pgte commented Dec 28, 2016

dignifiedquire Dec 29, 2016

pgte Dec 29, 2016

dignifiedquire Dec 29, 2016

pgte Dec 29, 2016

dignifiedquire commented Dec 29, 2016

dominictarr commented Dec 29, 2016

pgte commented Dec 30, 2016

daviddias left a comment

daviddias Dec 30, 2016

dignifiedquire Dec 30, 2016

dignifiedquire Dec 30, 2016

dignifiedquire Dec 30, 2016

dignifiedquire Dec 30, 2016

dignifiedquire Dec 30, 2016

dignifiedquire Dec 30, 2016

daviddias Dec 30, 2016

dignifiedquire Dec 30, 2016

dignifiedquire Dec 30, 2016

pgte commented Jan 3, 2017

pgte commented Jan 4, 2017

daviddias Jan 9, 2017

daviddias Jan 9, 2017

daviddias Jan 9, 2017

daviddias commented Jan 11, 2017


		const result = pushable()

		reduceToParents(source, function (err, roots) {

Builder refactoring, trickle builder and balanced builder #118

Builder refactoring, trickle builder and balanced builder #118

Conversation

pgte commented Dec 28, 2016

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

dignifiedquire commented Dec 29, 2016

dominictarr commented Dec 29, 2016

pgte commented Dec 30, 2016

daviddias 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

pgte commented Jan 3, 2017

pgte commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Jan 11, 2017