Skip to content
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

Do not merge: Spec for adder APIs #1321

Closed
wants to merge 7 commits into from
Closed

Do not merge: Spec for adder APIs #1321

wants to merge 7 commits into from

Conversation

wking
Copy link
Contributor

@wking wking commented Jun 2, 2015

Spun off from #1291, since I wanted a way to track the spec's
evolution and allow for inline comments. This PR has the spec in an
orphan branch to support that, but once we settle on a spec we'll just
grab the bits from this file and incorporate them in interface
definitions in the master branch (instead of merging this branch).

wking added 5 commits June 2, 2015 09:53
This will let us handle functionality like ignored files (#1232)
without needing to alter the adder API or chunk/trickle files that we
know we're going to ignore ahead of time.

I considered sticking with nOut *dag.Node instead of ignore bool to
give folks more flexibility (e.g. "use nOut instead of
chunking/trickling that path yourself"), but I can't think of an easy
way to distinguish "please ignore this path" from "please handle this
path using your usual chunking/trickling" if we're returning just nOut
and err. So this callback just returns a Boolean distinguishing
"please ignore" from "please chunk/trickle as usual", and we can add
an additional nOut return if we find a use case for a
callback-supplied substitute node.
On Wed, May 27, 2015 at 10:44:32AM -0700, Jeromy Johnson wrote [1]:
> ... the functionality you are referring to as 'trickle' is the
> layout. the trickledag is a specific layout implementation that lays
> out blocks in a pattern reminiscent of a recursive binomial heap.

[1]: #1291 (comment)
On Mon, Jun 01, 2015 at 07:29:16PM -0700, Juan Batiz-Benet wrote [1]:
> - name the parameters full words. single char var names in the doc
>   are a bit hard to read.

The only parameter that's not a full word is 'ctx', since that seems
to be the conventional name for the Context parameter.

[1]: #1291 (comment)
This allows you to easily pass in arrays of independent callbacks.
For example, you might check several conditions to decide whether or
not to ignore a file, and you can handle each condition in an
independent preNodeCallBack.  Or you might want to wrap your file
nodes in metadata nodes, and then print progress on those metadata
nodes to the terminal.  One tricky bit about the post-node callback
slice is that with the single-callback form the callback was called
with every newly-created node.  But with the new logic you can have:

1. Add creates node1.
2. First callback wraps node1 in node2 (so node2 links node1).
3. Second callback acts on node2, but is never called for node1.

However you could also have:

1. Add creates node1.
2. First callback drops node1 and replaced it with node2 (so node2
   does *not* depend on node1).
3. Second callback acts on node2, and is never called for node1, as it
   should be.

And you could also have:

1. Add creates node1.
2. First callback wraps node1 in node2 (so node2 links node1), and
   also creates a new node3 that is linked from node2:

     node2
     |-- node1
     `-- node3

3. Second callback acts on node2, but is never called for node1 or
   node3.

The only way I can think of to cleanly handle this case is to have the
postNodeCallback return a slice of new nodes, with the first node in
the slice being the new tip, and the subsequent nodes just being
passed on to the subsequent callbacks with top always unset.  That's
ugly enough that I'm putting it off for now ;).

We didn't have this problem with the previous single-callback
approach, but then you'd need a wrapping function or stateful object
to manage the union of callbacks in all cases.  With the current
implementation, folks can handle simple cases using the array of
callbacks, and fall back to creating a multi-callback wrapper when the
current iteration logic falls down for them.
wking added 2 commits June 3, 2015 12:57
This reverts commit d617dfb.

On Wed, Jun 03, 2015 at 01:54:03AM -0700, Juan Batiz-Benet wrote [1]:
> > it's probably worth passing an array of NodeCallback (and
> > PreNodeCallback) references to the adders
>
> i think this is way overkill. one can chain the calls with one
> function themselves. you might even provide a utility function that
> turns N callbacks into one which applies them sequentially, but this
> function should just accept one callback.

That's a much cleaner approach, so we're going back to the non-slice
callbacks.

[1]: #1291 (comment)
Juan argued for this rename because he expects the Reader-based case
will be more common in a highly-networked world [1].  I'd rather have
kept the old naming, because I expect some folks will think AddFile is
only about leaf files (when it also handles directories and recursion
via an *os.File file descriptor) [2].  But Juan still felt that the
Reader-case deserved top billing [3], so go with his suggested naming.

[1]: #1291 (comment)
[2]: #1291 (comment)
[3]: #1291 (comment)
@jbenet
Copy link
Member

jbenet commented Jun 30, 2015

@wking do we still need this PR? can we close and reopen when needed?

@jbenet jbenet closed this Jul 22, 2015
@jbenet jbenet removed the status/in-progress In progress label Jul 22, 2015
@Kubuxu Kubuxu deleted the add-api branch February 27, 2017 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants