-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
+25
−18
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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)
@wking do we still need this PR? can we close and reopen when needed? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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).