[BEAM-3287] Use model pipelines in Go SDK#4213
Conversation
herohde
commented
Dec 5, 2017
herohde
commented
- Replace existing harness bundle translation.
- Also stage model pipeline for Dataflow.
lostluck
left a comment
lostluck
left a comment
There was a problem hiding this comment.
I clearly have a bunch to say :P. This is as far as I've currently got time for.
| } | ||
|
|
||
| // Compute the input/output for this scope: | ||
| // inputs := U(input)\U(outputs) |
There was a problem hiding this comment.
U for "union"? It took me longer than necessary to figure that out. The \ is for intersection?
I'm not sure what the difference is between the singular input and output from the outputs and the inputs, and it seems that we're recomputing them?
|
|
||
| // TODO(herohde) 11/6/2017: move some of the configuration into the graph during construction. | ||
|
|
||
| // Options |
|
|
||
| // SDK constants | ||
|
|
||
| urnDoFn = "urn:beam:dofn:javasdk:0.1" // TODO: use "urn:beam:go:transform:dofn:v1" |
There was a problem hiding this comment.
Consider commenting under which circumstances the change would be made. It's not clear to a reader why one wouldn't just use this now.
| const ( | ||
| // Model constants | ||
|
|
||
| urnImpulse = "urn:beam:transform:impulse:v1" // to appear? |
There was a problem hiding this comment.
rm " // to appear?" It doesn't add anything meaningful to the reader, let alone suggest anything that would need to change if it has or has not been implemented in the model.
Instead link/reference to the appropriate Jira proposal if it's only that far in.
Otherwise it's not worth calling attention to it.
| return id | ||
| } | ||
|
|
||
| var sub []string |
There was a problem hiding this comment.
sub is a list of edges? I don't know what the sub list is for from the context though, substituting? subtraction? sublimation?
| return id | ||
| } | ||
|
|
||
| var sub []string |
There was a problem hiding this comment.
lostluck wrote:
sub is a list of edges? I don't know what the sub list is for from the context though, substituting? subtraction? sublimation?
Changed to subtransforms.
| } | ||
|
|
||
| // Compute the input/output for this scope: | ||
| // inputs := U(input)\U(outputs) |
There was a problem hiding this comment.
lostluck wrote:
U for "union"? It took me longer than necessary to figure that out. The \ is for intersection?
I'm not sure what the difference is between the singular input and output from the outputs and the inputs, and it seems that we're recomputing them?
Updated the comment. We're not recomputing them, just converting them to a map to support the set operations more easily.
| const ( | ||
| // Model constants | ||
|
|
||
| urnImpulse = "urn:beam:transform:impulse:v1" // to appear? |
There was a problem hiding this comment.
lostluck wrote:
rm " // to appear?" It doesn't add anything meaningful to the reader, let alone suggest anything that would need to change if it has or has not been implemented in the model.Instead link/reference to the appropriate Jira proposal if it's only that far in.
Otherwise it's not worth calling attention to it.
Done.
|
|
||
| // SDK constants | ||
|
|
||
| urnDoFn = "urn:beam:dofn:javasdk:0.1" // TODO: use "urn:beam:go:transform:dofn:v1" |
There was a problem hiding this comment.
lostluck wrote:
Consider commenting under which circumstances the change would be made. It's not clear to a reader why one wouldn't just use this now.
Done.
|
|
||
| // TODO(herohde) 11/6/2017: move some of the configuration into the graph during construction. | ||
|
|
||
| // Options |
There was a problem hiding this comment.
lostluck wrote:
Options for... ?
Done.
* Remove the old translator, which had problematic tests w/ obsolete source/sink.
| return e.id | ||
| } | ||
|
|
||
| // Name returns a not-necessarily-unique name for the edge. |
There was a problem hiding this comment.
It's not clear to me as a user of this API what I'm supposed to do with this info.
There was a problem hiding this comment.
Know that the name isn't unique, which is a requirement in various contexts. The tree.go code precisely helps make these transform names unique.
|
|
||
| // ScopeTree is a convenient representation of the Scope-structure as a tree. | ||
| // Each ScopeTree may also be a subtree of another ScopeTree. The tree structure | ||
| // must respect the underlying Scope structure, i.e., if Scope 'a' has a parent |
There was a problem hiding this comment.
The tree structure respects the underlying... This is describing a property of treeStructure, but the "must respect" makes it sound like it's requirement on the user of this API.
|
Thanks! |
|
R: @kennknowles (merge) |
|
Great. And the irrelevant Java failure appears to be |
4 participants
